Merge branch 'orderable-issues' into 'master'

Allow issues in boards to be ordered

Closes #21264

See merge request !8916

app/assets/javascripts/boards/components/board_list.js

@@ -56,11 +56,6 @@ import boardCard from './board_card';
         });
       }
     },
-    computed: {
-      orderedIssues () {
-        return _.sortBy(this.issues, 'priority');
-      },
-    },
     methods: {
       listHeight () {
         return this.$refs.list.getBoundingClientRect().height;
@@ -92,9 +87,9 @@ import boardCard from './board_card';
       const options = gl.issueBoards.getBoardSortableDefaultOptions({
         scroll: document.querySelectorAll('.boards-list')[0],
         group: 'issues',
-        sort: false,
         disabled: this.disabled,
         filter: '.board-list-count, .is-disabled',
+        dataIdAttr: 'data-issue-id',
         onStart: (e) => {
           const card = this.$refs.issue[e.oldIndex];
 
@@ -111,6 +106,13 @@ import boardCard from './board_card';
             e.item.remove();
           });
         },
+        onUpdate: (e) => {
+          const sortedArray = this.sortable.toArray().filter(id => id !== '-1');
+          gl.issueBoards.BoardsStore.moveIssueInList(this.list, Store.moving.issue, e.oldIndex, e.newIndex, sortedArray);
+        },
+        onMove(e) {
+          return !e.related.classList.contains('board-list-count');
+        }
       });
 
       this.sortable = Sortable.create(this.$refs.list, options);

app/assets/javascripts/boards/models/issue.js

@@ -15,6 +15,7 @@ class ListIssue {
     this.labels = [];
     this.selected = false;
     this.assignee = false;
+    this.position = obj.relative_position || Infinity;
 
     if (obj.assignee) {
       this.assignee = new ListUser(obj.assignee);
@@ -27,10 +28,6 @@ class ListIssue {
     obj.labels.forEach((label) => {
       this.labels.push(new ListLabel(label));
     });
-
-    this.priority = this.labels.reduce((max, label) => {
-      return (label.priority < max) ? label.priority : max;
-    }, Infinity);
   }
 
   addLabel (label) {

app/assets/javascripts/boards/models/list.js

@@ -110,9 +110,20 @@ class List {
   }
 
   addIssue (issue, listFrom, newIndex) {
+    let moveBeforeIid = null;
+    let moveAfterIid = null;
+
     if (!this.findIssue(issue.id)) {
       if (newIndex !== undefined) {
         this.issues.splice(newIndex, 0, issue);
+
+        if (this.issues[newIndex - 1]) {
+          moveBeforeIid = this.issues[newIndex - 1].id;
+        }
+
+        if (this.issues[newIndex + 1]) {
+          moveAfterIid = this.issues[newIndex + 1].id;
+        }
       } else {
         this.issues.push(issue);
       }
@@ -123,13 +134,21 @@ class List {
 
       if (listFrom) {
         this.issuesSize += 1;
-        this.updateIssueLabel(issue, listFrom);
+
+        this.updateIssueLabel(issue, listFrom, moveBeforeIid, moveAfterIid);
       }
     }
   }
 
-  updateIssueLabel(issue, listFrom) {
-    gl.boardService.moveIssue(issue.id, listFrom.id, this.id)
+  moveIssue (issue, oldIndex, newIndex, moveBeforeIid, moveAfterIid) {
+    this.issues.splice(oldIndex, 1);
+    this.issues.splice(newIndex, 0, issue);
+
+    gl.boardService.moveIssue(issue.id, null, null, moveBeforeIid, moveAfterIid);
+  }
+
+  updateIssueLabel(issue, listFrom, moveBeforeIid, moveAfterIid) {
+    gl.boardService.moveIssue(issue.id, listFrom.id, this.id, moveBeforeIid, moveAfterIid)
       .then(() => {
         listFrom.getIssues(false);
       });

app/assets/javascripts/boards/services/board_service.js

@@ -64,10 +64,12 @@ class BoardService {
     return this.issues.get(data);
   }
 
-  moveIssue (id, from_list_id, to_list_id) {
+  moveIssue (id, from_list_id = null, to_list_id = null, move_before_iid = null, move_after_iid = null) {
     return this.issue.update({ id }, {
       from_list_id,
-      to_list_id
+      to_list_id,
+      move_before_iid,
+      move_after_iid,
     });
   }
 

app/assets/javascripts/boards/stores/boards_store.js

@@ -109,6 +109,12 @@
         listFrom.removeIssue(issue);
       }
     },
+    moveIssueInList (list, issue, oldIndex, newIndex, idArray) {
+      const beforeId = parseInt(idArray[newIndex - 1], 10) || null;
+      const afterId = parseInt(idArray[newIndex + 1], 10) || null;
+
+      list.moveIssue(issue, oldIndex, newIndex, beforeId, afterId);
+    },
     findList (key, val, type = 'label') {
       return this.state.lists.filter((list) => {
         const byType = type ? list['type'] === type : true;

app/assets/javascripts/test_utils/simulate_drag.js

@@ -43,7 +43,14 @@
     return event;
   }
 
-  function getTraget(target) {
+  function isLast(target) {
+    var el = typeof target.el === 'string' ? document.getElementById(target.el.substr(1)) : target.el;
+    var children = el.children;
+
+    return children.length - 1 === target.index;
+  }
+
+  function getTarget(target) {
     var el = typeof target.el === 'string' ? document.getElementById(target.el.substr(1)) : target.el;
     var children = el.children;
 
@@ -75,12 +82,22 @@
   function simulateDrag(options, callback) {
     options.to.el = options.to.el || options.from.el;
 
-    var fromEl = getTraget(options.from);
-    var toEl = getTraget(options.to);
+    var fromEl = getTarget(options.from);
+    var toEl = getTarget(options.to);
+    var firstEl = getTarget({
+      el: options.to.el,
+      index: 'first'
+    });
+    var lastEl = getTarget({
+      el: options.to.el,
+      index: 'last'
+    });
     var scrollable = options.scrollable;
 
     var fromRect = getRect(fromEl);
     var toRect = getRect(toEl);
+    var firstRect = getRect(firstEl);
+    var lastRect = getRect(lastEl);
 
     var startTime = new Date().getTime();
     var duration = options.duration || 1000;
@@ -88,6 +105,12 @@
     options.ontap && options.ontap();
     window.SIMULATE_DRAG_ACTIVE = 1;
 
+    if (options.to.index === 0) {
+      toRect.cy = firstRect.y;
+    } else if (isLast(options.to)) {
+      toRect.cy = lastRect.y + lastRect.h + 50;
+    }
+
     var dragInterval = setInterval(function loop() {
       var progress = (new Date().getTime() - startTime) / duration;
       var x = (fromRect.cx + (toRect.cx - fromRect.cx) * progress) - scrollable.scrollLeft;

app/controllers/projects/boards/issues_controller.rb

@@ -8,6 +8,7 @@ module Projects
       def index
         issues = ::Boards::Issues::ListService.new(project, current_user, filter_params).execute
         issues = issues.page(params[:page]).per(params[:per] || 20)
+        make_sure_position_is_set(issues)
 
         render json: {
           issues: serialize_as_json(issues),
@@ -38,6 +39,12 @@ module Projects
 
       private
 
+      def make_sure_position_is_set(issues)
+        issues.each do |issue|
+          issue.move_to_end && issue.save unless issue.relative_position
+        end
+      end
+
       def issue
         @issue ||=
           IssuesFinder.new(current_user, project_id: project.id)
@@ -63,7 +70,7 @@ module Projects
       end
 
       def move_params
-        params.permit(:board_id, :id, :from_list_id, :to_list_id)
+        params.permit(:board_id, :id, :from_list_id, :to_list_id, :move_before_iid, :move_after_iid)
       end
 
       def issue_params
@@ -73,7 +80,7 @@ module Projects
       def serialize_as_json(resource)
         resource.as_json(
           labels: true,
-          only: [:id, :iid, :title, :confidential, :due_date],
+          only: [:id, :iid, :title, :confidential, :due_date, :relative_position],
           include: {
             assignee: { only: [:id, :name, :username], methods: [:avatar_url] },
             milestone: { only: [:id, :title] }

app/models/concerns/relative_positioning.rb

+module RelativePositioning
+  extend ActiveSupport::Concern
+
+  MIN_POSITION = 0
+  MAX_POSITION = Gitlab::Database::MAX_INT_VALUE
+
+  included do
+    after_save :save_positionable_neighbours
+  end
+
+  def min_relative_position
+    self.class.in_projects(project.id).minimum(:relative_position)
+  end
+
+  def max_relative_position
+    self.class.in_projects(project.id).maximum(:relative_position)
+  end
+
+  def prev_relative_position
+    prev_pos = nil
+
+    if self.relative_position
+      prev_pos = self.class.
+        in_projects(project.id).
+        where('relative_position < ?', self.relative_position).
+        maximum(:relative_position)
+    end
+
+    prev_pos || MIN_POSITION
+  end
+
+  def next_relative_position
+    next_pos = nil
+
+    if self.relative_position
+      next_pos = self.class.
+        in_projects(project.id).
+        where('relative_position > ?', self.relative_position).
+        minimum(:relative_position)
+    end
+
+    next_pos || MAX_POSITION
+  end
+
+  def move_between(before, after)
+    return move_after(before) unless after
+    return move_before(after) unless before
+
+    pos_before = before.relative_position
+    pos_after = after.relative_position
+
+    if pos_after && (pos_before == pos_after)
+      self.relative_position = pos_before
+      before.move_before(self)
+      after.move_after(self)
+
+      @positionable_neighbours = [before, after]
+    else
+      self.relative_position = position_between(pos_before, pos_after)
+    end
+  end
+
+  def move_before(after)
+    self.relative_position = position_between(after.prev_relative_position, after.relative_position)
+  end
+
+  def move_after(before)
+    self.relative_position = position_between(before.relative_position, before.next_relative_position)
+  end
+
+  def move_to_end
+    self.relative_position = position_between(max_relative_position, MAX_POSITION)
+  end
+
+  private
+
+  # This method takes two integer values (positions) and
+  # calculates some random position between them. The range is huge as
+  # the maximum integer value is 2147483647. Ideally, the calculated value would be
+  # exactly between those terminating values, but this will introduce possibility of a race condition
+  # so two or more issues can get the same value, we want to avoid that and we also want to avoid
+  # using a lock here. If we have two issues with distance more than one thousand, we are OK.
+  # Given the huge range of possible values that integer can fit we shoud never face a problem.
+  def position_between(pos_before, pos_after)
+    pos_before ||= MIN_POSITION
+    pos_after ||= MAX_POSITION
+
+    pos_before, pos_after = [pos_before, pos_after].sort
+
+    rand(pos_before.next..pos_after.pred)
+  end
+
+  def save_positionable_neighbours
+    return unless @positionable_neighbours
+
+    status = @positionable_neighbours.all?(&:save)
+    @positionable_neighbours = nil
+
+    status
+  end
+end

app/models/issue.rb

@@ -7,6 +7,7 @@ class Issue < ActiveRecord::Base
   include Sortable
   include Spammable
   include FasterCacheKeys
+  include RelativePositioning
 
   DueDateStruct = Struct.new(:title, :name).freeze
   NoDueDate     = DueDateStruct.new('No Due Date', '0').freeze

app/services/boards/issues/list_service.rb

@@ -5,7 +5,7 @@ module Boards
         issues = IssuesFinder.new(current_user, filter_params).execute
         issues = without_board_labels(issues) unless movable_list?
         issues = with_list_label(issues) if movable_list?
-        issues
+        issues.reorder(Gitlab::Database.nulls_last_order('relative_position', 'ASC'))
       end
 
       private
@@ -26,7 +26,6 @@ module Boards
 
       def filter_params
         set_default_scope
-        set_default_sort
         set_project
         set_state
 
@@ -37,10 +36,6 @@ module Boards
         params[:scope] = 'all'
       end
 
-      def set_default_sort
-        params[:sort] = 'priority'
-      end
-
       def set_project
         params[:project_id] = project.id
       end

app/services/boards/issues/move_service.rb

@@ -3,7 +3,7 @@ module Boards
     class MoveService < BaseService
       def execute(issue)
         return false unless can?(current_user, :update_issue, issue)
-        return false unless valid_move?
+        return false if issue_params.empty?
 
         update_service.execute(issue)
       end
@@ -14,7 +14,7 @@ module Boards
         @board ||= project.boards.find(params[:board_id])
       end
 
-      def valid_move?
+      def move_between_lists?
         moving_from_list.present? && moving_to_list.present? &&
           moving_from_list != moving_to_list
       end
@@ -32,11 +32,19 @@ module Boards
       end
 
       def issue_params
-        {
-          add_label_ids: add_label_ids,
-          remove_label_ids: remove_label_ids,
-          state_event: issue_state
-        }
+        attrs = {}
+
+        if move_between_lists?
+          attrs.merge!(
+            add_label_ids: add_label_ids,
+            remove_label_ids: remove_label_ids,
+            state_event: issue_state,
+          )
+        end
+
+        attrs[:move_between_iids] = move_between_iids if move_between_iids
+
+        attrs
       end
 
       def issue_state
@@ -58,6 +66,12 @@ module Boards
 
         Array(label_ids).compact
       end
+
+      def move_between_iids
+        return unless params[:move_after_iid] || params[:move_before_iid]
+
+        [params[:move_after_iid], params[:move_before_iid]]
+      end
     end
   end
 end

app/services/issuable_base_service.rb

@@ -211,7 +211,7 @@ class IssuableBaseService < BaseService
     label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids)
     params[:label_ids] = label_ids if labels_changing?(issuable.label_ids, label_ids)
 
-    if params.present?
+    if issuable.changed? || params.present?
       issuable.assign_attributes(params.merge(updated_by: current_user))
 
       before_update(issuable)

app/services/issues/create_service.rb

@@ -13,6 +13,7 @@ module Issues
 
     def before_create(issue)
       spam_check(issue, current_user)
+      issue.move_to_end
     end
 
     def after_create(issuable)

app/services/issues/update_service.rb

@@ -3,8 +3,8 @@ module Issues
     include SpamCheckService
 
     def execute(issue)
+      handle_move_between_iids(issue)
       filter_spam_check_params
-
       update(issue)
     end
 
@@ -37,11 +37,13 @@ module Issues
       end
 
       added_labels = issue.labels - old_labels
+
       if added_labels.present?
         notification_service.relabeled_issue(issue, added_labels, current_user)
       end
 
       added_mentions = issue.mentioned_users - old_mentioned_users
+
       if added_mentions.present?
         notification_service.new_mentions_in_issue(issue, added_mentions, current_user)
       end
@@ -55,8 +57,24 @@ module Issues
       Issues::CloseService
     end
 
+    def handle_move_between_iids(issue)
+      return unless params[:move_between_iids]
+
+      after_iid, before_iid = params.delete(:move_between_iids)
+
+      issue_before = get_issue_if_allowed(issue.project, before_iid) if before_iid
+      issue_after = get_issue_if_allowed(issue.project, after_iid) if after_iid
+
+      issue.move_between(issue_before, issue_after)
+    end
+
     private
 
+    def get_issue_if_allowed(project, iid)
+      issue = project.issues.find_by(iid: iid)
+      issue if can?(current_user, :update_issue, issue)
+    end
+
     def create_confidentiality_note(issue)
       SystemNoteService.change_issue_confidentiality(issue, issue.project, current_user)
     end

app/views/projects/boards/components/_board_list.html.haml

@@ -8,7 +8,7 @@
     "v-show" => "!loading",
     ":data-board" => "list.id",
     ":class" => '{ "is-smaller": showIssueForm }' }
-    %board-card{ "v-for" => "(issue, index) in orderedIssues",
+    %board-card{ "v-for" => "(issue, index) in issues",
       "ref" => "issue",
       ":index" => "index",
       ":list" => "list",
@@ -17,7 +17,8 @@
       ":root-path" => "rootPath",
       ":disabled" => "disabled",
       ":key" => "issue.id" }
-    %li.board-list-count.text-center{ "v-if" => "showCount" }
+    %li.board-list-count.text-center{ "v-if" => "showCount",
+      "data-issue-id" => "-1" }
       = icon("spinner spin", "v-show" => "list.loadingMore" )
       %span{ "v-if" => "list.issues.length === list.issuesSize" }
         Showing all issues

db/migrate/20170131221752_add_relative_position_to_issues.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddRelativePositionToIssues < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  # When a migration requires downtime you **must** uncomment the following
+  # constant and define a short and easy to understand explanation as to why the
+  # migration requires downtime.
+  # DOWNTIME_REASON = ''
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  disable_ddl_transaction!
+
+  def up
+    add_column :issues, :relative_position, :integer
+
+    add_concurrent_index :issues, :relative_position
+  end
+
+  def down
+    remove_column :issues, :relative_position
+
+    remove_index :issues, :relative_position if index_exists? :issues, :relative_position
+  end
+end

db/schema.rb

@@ -530,6 +530,7 @@ ActiveRecord::Schema.define(version: 20170306170512) do
     t.text "title_html"
     t.text "description_html"
     t.integer "time_estimate"
+    t.integer "relative_position"
   end
 
   add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
@@ -541,6 +542,7 @@ ActiveRecord::Schema.define(version: 20170306170512) do
   add_index "issues", ["due_date"], name: "index_issues_on_due_date", using: :btree
   add_index "issues", ["milestone_id"], name: "index_issues_on_milestone_id", using: :btree
   add_index "issues", ["project_id", "iid"], name: "index_issues_on_project_id_and_iid", unique: true, using: :btree
+  add_index "issues", ["relative_position"], name: "index_issues_on_relative_position", using: :btree
   add_index "issues", ["state"], name: "index_issues_on_state", using: :btree
   add_index "issues", ["title"], name: "index_issues_on_title_trigram", using: :gin, opclasses: {"title"=>"gin_trgm_ops"}
 

spec/controllers/projects/boards/issues_controller_spec.rb

@@ -43,6 +43,7 @@ describe Projects::Boards::IssuesController do
 
           expect(response).to match_response_schema('issues')
           expect(parsed_response.length).to eq 2
+          expect(development.issues.map(&:relative_position)).not_to include(nil)
         end
       end
 

spec/features/boards/boards_spec.rb

@@ -71,16 +71,16 @@ describe 'Issue Boards', feature: true, js: true do
     let!(:list1) { create(:list, board: board, label: planning, position: 0) }
     let!(:list2) { create(:list, board: board, label: development, position: 1) }
 
-    let!(:confidential_issue) { create(:labeled_issue, :confidential, project: project, author: user, labels: [planning]) }
-    let!(:issue1) { create(:labeled_issue, project: project, assignee: user, labels: [planning]) }
-    let!(:issue2) { create(:labeled_issue, project: project, author: user2, labels: [planning]) }
-    let!(:issue3) { create(:labeled_issue, project: project, labels: [planning]) }
-    let!(:issue4) { create(:labeled_issue, project: project, labels: [planning]) }
-    let!(:issue5) { create(:labeled_issue, project: project, labels: [planning], milestone: milestone) }
-    let!(:issue6) { create(:labeled_issue, project: project, labels: [planning, development]) }
-    let!(:issue7) { create(:labeled_issue, project: project, labels: [development]) }
+    let!(:confidential_issue) { create(:labeled_issue, :confidential, project: project, author: user, labels: [planning], relative_position: 9) }
+    let!(:issue1) { create(:labeled_issue, project: project, assignee: user, labels: [planning], relative_position: 8) }
+    let!(:issue2) { create(:labeled_issue, project: project, author: user2, labels: [planning], relative_position: 7) }
+    let!(:issue3) { create(:labeled_issue, project: project, labels: [planning], relative_position: 6) }
+    let!(:issue4) { create(:labeled_issue, project: project, labels: [planning], relative_position: 5) }
+    let!(:issue5) { create(:labeled_issue, project: project, labels: [planning], milestone: milestone, relative_position: 4) }
+    let!(:issue6) { create(:labeled_issue, project: project, labels: [planning, development], relative_position: 3) }
+    let!(:issue7) { create(:labeled_issue, project: project, labels: [development], relative_position: 2) }
     let!(:issue8) { create(:closed_issue, project: project) }
-    let!(:issue9) { create(:labeled_issue, project: project, labels: [planning, testing, bug, accepting]) }
+    let!(:issue9) { create(:labeled_issue, project: project, labels: [planning, testing, bug, accepting], relative_position: 1) }
 
     before do
       visit namespace_project_board_path(project.namespace, project, board)

spec/features/boards/issue_ordering_spec.rb

+require 'rails_helper'
+
+describe 'Issue Boards', :feature, :js do
+  include WaitForVueResource
+  include DragTo
+
+  let(:project) { create(:empty_project, :public) }
+  let(:board) { create(:board, project: project) }
+  let(:user) { create(:user) }
+  let(:label) { create(:label, project: project) }
+  let!(:list1) { create(:list, board: board, label: label, position: 0) }
+  let!(:issue1) { create(:labeled_issue, project: project, title: 'testing 1', labels: [label], relative_position: 3) }
+  let!(:issue2) { create(:labeled_issue, project: project, title: 'testing 2', labels: [label], relative_position: 2) }
+  let!(:issue3) { create(:labeled_issue, project: project, title: 'testing 3', labels: [label], relative_position: 1) }
+
+  before do
+    project.team << [user, :master]
+
+    login_as(user)
+  end
+
+  context 'un-ordered issues' do
+    let!(:issue4) { create(:labeled_issue, project: project, labels: [label]) }
+
+    before do
+      visit namespace_project_board_path(project.namespace, project, board)
+      wait_for_vue_resource
+
+      expect(page).to have_selector('.board', count: 2)
+    end
+
+    it 'has un-ordered issue as last issue' do
+      page.within(first('.board')) do
+        expect(all('.card').last).to have_content(issue4.title)
+      end
+    end
+
+    it 'moves un-ordered issue to top of list' do
+      drag(from_index: 3, to_index: 0)
+
+      page.within(first('.board')) do
+        expect(first('.card')).to have_content(issue4.title)
+      end
+    end
+  end
+
+  context 'ordering in list' do
+    before do
+      visit namespace_project_board_path(project.namespace, project, board)
+      wait_for_vue_resource
+
+      expect(page).to have_selector('.board', count: 2)
+    end
+
+    it 'moves from middle to top' do
+      drag(from_index: 1, to_index: 0)
+
+      wait_for_vue_resource
+
+      expect(first('.card')).to have_content(issue2.title)
+    end
+
+    it 'moves from middle to bottom' do
+      drag(from_index: 1, to_index: 2)
+
+      wait_for_vue_resource
+
+      expect(all('.card').last).to have_content(issue2.title)
+    end
+
+    it 'moves from top to bottom' do
+      drag(from_index: 0, to_index: 2)
+
+      wait_for_vue_resource
+
+      expect(all('.card').last).to have_content(issue3.title)
+    end
+
+    it 'moves from bottom to top' do
+      drag(from_index: 2, to_index: 0)
+
+      wait_for_vue_resource
+
+      expect(first('.card')).to have_content(issue1.title)
+    end
+
+    it 'moves from top to middle' do
+      drag(from_index: 0, to_index: 1)
+
+      wait_for_vue_resource
+
+      expect(first('.card')).to have_content(issue2.title)
+    end
+
+    it 'moves from bottom to middle' do
+      drag(from_index: 2, to_index: 1)
+
+      wait_for_vue_resource
+
+      expect(all('.card').last).to have_content(issue2.title)
+    end
+  end
+
+  context 'ordering when changing list' do
+    let(:label2) { create(:label, project: project) }
+    let!(:list2) { create(:list, board: board, label: label2, position: 1) }
+    let!(:issue4) { create(:labeled_issue, project: project, title: 'testing 1', labels: [label2], relative_position: 3.0) }
+    let!(:issue5) { create(:labeled_issue, project: project, title: 'testing 2', labels: [label2], relative_position: 2.0) }
+    let!(:issue6) { create(:labeled_issue, project: project, title: 'testing 3', labels: [label2], relative_position: 1.0) }
+
+    before do
+      visit namespace_project_board_path(project.namespace, project, board)
+      wait_for_vue_resource
+
+      expect(page).to have_selector('.board', count: 3)
+    end
+
+    it 'moves to top of another list' do
+      drag(list_from_index: 0, list_to_index: 1)
+
+      wait_for_vue_resource
+
+      expect(first('.board')).to have_selector('.card', count: 2)
+      expect(all('.board')[1]).to have_selector('.card', count: 4)
+
+      page.within(all('.board')[1]) do
+        expect(first('.card')).to have_content(issue3.title)
+      end
+    end
+
+    it 'moves to bottom of another list' do
+      drag(list_from_index: 0, list_to_index: 1, to_index: 2)
+
+      wait_for_vue_resource
+
+      expect(first('.board')).to have_selector('.card', count: 2)
+      expect(all('.board')[1]).to have_selector('.card', count: 4)
+
+      page.within(all('.board')[1]) do
+        expect(all('.card').last).to have_content(issue3.title)
+      end
+    end
+
+    it 'moves to index of another list' do
+      drag(list_from_index: 0, list_to_index: 1, to_index: 1)
+
+      wait_for_vue_resource
+
+      expect(first('.board')).to have_selector('.card', count: 2)
+      expect(all('.board')[1]).to have_selector('.card', count: 4)
+
+      page.within(all('.board')[1]) do
+        expect(all('.card')[1]).to have_content(issue3.title)
+      end
+    end
+  end
+
+  def drag(selector: '.board-list', list_from_index: 0, from_index: 0, to_index: 0, list_to_index: 0)
+    drag_to(selector: selector,
+            scrollable: '#board-app',
+            list_from_index: list_from_index,
+            from_index: from_index,
+            to_index: to_index,
+            list_to_index: list_to_index)
+  end
+end

spec/features/boards/sidebar_spec.rb

@@ -11,8 +11,8 @@ describe 'Issue Boards', feature: true, js: true do
   let!(:bug)         { create(:label, project: project, name: 'Bug') }
   let!(:regression)  { create(:label, project: project, name: 'Regression') }
   let!(:stretch)     { create(:label, project: project, name: 'Stretch') }
-  let!(:issue1)      { create(:labeled_issue, project: project, assignee: user, milestone: milestone, labels: [development]) }
-  let!(:issue2)      { create(:labeled_issue, project: project, labels: [development, stretch]) }
+  let!(:issue1)      { create(:labeled_issue, project: project, assignee: user, milestone: milestone, labels: [development], relative_position: 2) }
+  let!(:issue2)      { create(:labeled_issue, project: project, labels: [development, stretch], relative_position: 1) }
   let(:board)        { create(:board, project: project) }
   let!(:list)        { create(:list, board: board, label: development, position: 0) }
   let(:card) { first('.board').first('.card') }

spec/fixtures/api/schemas/issue.json

@@ -11,6 +11,7 @@
     "title": { "type": "string" },
     "confidential": { "type": "boolean" },
     "due_date": { "type": ["date", "null"] },
+    "relative_position": { "type": "integer" },
     "labels": {
       "type": "array",
       "items": {

spec/javascripts/boards/boards_store_spec.js

@@ -5,6 +5,7 @@
 /* global Cookies */
 /* global listObj */
 /* global listObjDuplicate */
+/* global ListIssue */
 
 require('~/lib/utils/url_utility');
 require('~/boards/models/issue');
@@ -14,6 +15,7 @@ require('~/boards/models/user');
 require('~/boards/services/board_service');
 require('~/boards/stores/boards_store');
 require('./mock_data');
+require('es6-promise').polyfill();
 
 describe('Store', () => {
   beforeEach(() => {
@@ -21,6 +23,10 @@ describe('Store', () => {
     gl.boardService = new BoardService('/test/issue-boards/board', '', '1');
     gl.issueBoards.BoardsStore.create();
 
+    spyOn(gl.boardService, 'moveIssue').and.callFake(() => new Promise((resolve) => {
+      resolve();
+    }));
+
     Cookies.set('issue_board_welcome_hidden', 'false', {
       expires: 365 * 10,
       path: ''
@@ -154,5 +160,74 @@ describe('Store', () => {
         done();
       }, 0);
     });
+
+    it('moves issue to top of another list', (done) => {
+      const listOne = gl.issueBoards.BoardsStore.addList(listObj);
+      const listTwo = gl.issueBoards.BoardsStore.addList(listObjDuplicate);
+
+      expect(gl.issueBoards.BoardsStore.state.lists.length).toBe(2);
+
+      setTimeout(() => {
+        listOne.issues[0].id = 2;
+
+        expect(listOne.issues.length).toBe(1);
+        expect(listTwo.issues.length).toBe(1);
+
+        gl.issueBoards.BoardsStore.moveIssueToList(listOne, listTwo, listOne.findIssue(2), 0);
+
+        expect(listOne.issues.length).toBe(0);
+        expect(listTwo.issues.length).toBe(2);
+        expect(listTwo.issues[0].id).toBe(2);
+        expect(gl.boardService.moveIssue).toHaveBeenCalledWith(2, listOne.id, listTwo.id, null, 1);
+
+        done();
+      }, 0);
+    });
+
+    it('moves issue to bottom of another list', (done) => {
+      const listOne = gl.issueBoards.BoardsStore.addList(listObj);
+      const listTwo = gl.issueBoards.BoardsStore.addList(listObjDuplicate);
+
+      expect(gl.issueBoards.BoardsStore.state.lists.length).toBe(2);
+
+      setTimeout(() => {
+        listOne.issues[0].id = 2;
+
+        expect(listOne.issues.length).toBe(1);
+        expect(listTwo.issues.length).toBe(1);
+
+        gl.issueBoards.BoardsStore.moveIssueToList(listOne, listTwo, listOne.findIssue(2), 1);
+
+        expect(listOne.issues.length).toBe(0);
+        expect(listTwo.issues.length).toBe(2);
+        expect(listTwo.issues[1].id).toBe(2);
+        expect(gl.boardService.moveIssue).toHaveBeenCalledWith(2, listOne.id, listTwo.id, 1, null);
+
+        done();
+      }, 0);
+    });
+
+    it('moves issue in list', (done) => {
+      const issue = new ListIssue({
+        title: 'Testing',
+        iid: 2,
+        confidential: false,
+        labels: []
+      });
+      const list = gl.issueBoards.BoardsStore.addList(listObj);
+
+      setTimeout(() => {
+        list.addIssue(issue);
+
+        expect(list.issues.length).toBe(2);
+
+        gl.issueBoards.BoardsStore.moveIssueInList(list, issue, 0, 1, [1, 2]);
+
+        expect(list.issues[0].id).toBe(2);
+        expect(gl.boardService.moveIssue).toHaveBeenCalledWith(2, null, null, 1, null);
+
+        done();
+      });
+    });
   });
 });

spec/javascripts/boards/issue_spec.js

@@ -79,4 +79,20 @@ describe('Issue model', () => {
     issue.removeLabels([issue.labels[0], issue.labels[1]]);
     expect(issue.labels.length).toBe(0);
   });
+
+  it('sets position to infinity if no position is stored', () => {
+    expect(issue.position).toBe(Infinity);
+  });
+
+  it('sets position', () => {
+    const relativePositionIssue = new ListIssue({
+      title: 'Testing',
+      iid: 1,
+      confidential: false,
+      relative_position: 1,
+      labels: []
+    });
+
+    expect(relativePositionIssue.position).toBe(1);
+  });
 });

spec/javascripts/boards/list_spec.js

@@ -103,6 +103,7 @@ describe('List model', () => {
 
     listDup.updateIssueLabel(list, issue);
 
-    expect(gl.boardService.moveIssue).toHaveBeenCalledWith(issue.id, list.id, listDup.id);
+    expect(gl.boardService.moveIssue)
+      .toHaveBeenCalledWith(issue.id, list.id, listDup.id, undefined, undefined);
   });
 });

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -21,6 +21,7 @@ Issue:
 - milestone_id
 - weight
 - time_estimate
+- relative_position
 Event:
 - id
 - target_type

spec/models/concerns/relative_positioning_spec.rb

+require 'spec_helper'
+
+describe Issue, 'RelativePositioning' do
+  let(:project) { create(:empty_project) }
+  let(:issue) { create(:issue, project: project) }
+  let(:issue1) { create(:issue, project: project) }
+  let(:new_issue) { create(:issue, project: project) }
+
+  before do
+    [issue, issue1].each do |issue|
+      issue.move_to_end && issue.save
+    end
+  end
+
+  describe '#min_relative_position' do
+    it 'returns maximum position' do
+      expect(issue.min_relative_position).to eq issue.relative_position
+    end
+  end
+
+  describe '#max_relative_position' do
+    it 'returns maximum position' do
+      expect(issue.max_relative_position).to eq issue1.relative_position
+    end
+  end
+
+  describe '#prev_relative_position' do
+    it 'returns previous position if there is an issue above' do
+      expect(issue1.prev_relative_position).to eq issue.relative_position
+    end
+
+    it 'returns minimum position if there is no issue above' do
+      expect(issue.prev_relative_position).to eq RelativePositioning::MIN_POSITION
+    end
+  end
+
+  describe '#next_relative_position' do
+    it 'returns next position if there is an issue below' do
+      expect(issue.next_relative_position).to eq issue1.relative_position
+    end
+
+    it 'returns next position if there is no issue below' do
+      expect(issue1.next_relative_position).to eq RelativePositioning::MAX_POSITION
+    end
+  end
+
+  describe '#move_before' do
+    it 'moves issue before' do
+      [issue1, issue].each(&:move_to_end)
+
+      issue.move_before(issue1)
+
+      expect(issue.relative_position).to be < issue1.relative_position
+    end
+  end
+
+  describe '#move_after' do
+    it 'moves issue after' do
+      [issue, issue1].each(&:move_to_end)
+
+      issue.move_after(issue1)
+
+      expect(issue.relative_position).to be > issue1.relative_position
+    end
+  end
+
+  describe '#move_to_end' do
+    it 'moves issue to the end' do
+      new_issue.move_to_end
+
+      expect(new_issue.relative_position).to be > issue1.relative_position
+    end
+  end
+
+  describe '#move_between' do
+    it 'positions issue between two other' do
+      new_issue.move_between(issue, issue1)
+
+      expect(new_issue.relative_position).to be > issue.relative_position
+      expect(new_issue.relative_position).to be < issue1.relative_position
+    end
+
+    it 'positions issue between on top' do
+      new_issue.move_between(nil, issue)
+
+      expect(new_issue.relative_position).to be < issue.relative_position
+    end
+
+    it 'positions issue between to end' do
+      new_issue.move_between(issue1, nil)
+
+      expect(new_issue.relative_position).to be > issue1.relative_position
+    end
+
+    it 'positions issues even when after and before positions are the same' do
+      issue1.update relative_position: issue.relative_position
+
+      new_issue.move_between(issue, issue1)
+
+      expect(new_issue.relative_position).to be > issue.relative_position
+      expect(issue.relative_position).to be < issue1.relative_position
+    end
+  end
+end

spec/services/boards/issues/list_service_spec.rb

@@ -43,32 +43,6 @@ describe Boards::Issues::ListService, services: true do
       described_class.new(project, user, params).execute
     end
 
-    context 'sets default order to priority' do
-      it 'returns opened issues when list id is missing' do
-        params = { board_id: board.id }
-
-        issues = described_class.new(project, user, params).execute
-
-        expect(issues).to eq [opened_issue2, reopened_issue1, opened_issue1]
-      end
-
-      it 'returns closed issues when listing issues from Done' do
-        params = { board_id: board.id, id: done.id }
-
-        issues = described_class.new(project, user, params).execute
-
-        expect(issues).to eq [closed_issue4, closed_issue2, closed_issue3, closed_issue1]
-      end
-
-      it 'returns opened issues that have label list applied when listing issues from a label list' do
-        params = { board_id: board.id, id: list1.id }
-
-        issues = described_class.new(project, user, params).execute
-
-        expect(issues).to eq [list1_issue3, list1_issue1, list1_issue2]
-      end
-    end
-
     context 'with list that does not belong to the board' do
       it 'raises an error' do
         list = create(:list)

spec/services/boards/issues/move_service_spec.rb

@@ -78,8 +78,10 @@ describe Boards::Issues::MoveService, services: true do
     end
 
     context 'when moving to same list' do
-      let(:issue)  { create(:labeled_issue, project: project, labels: [bug, development]) }
-      let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list1.id } }
+      let(:issue)   { create(:labeled_issue, project: project, labels: [bug, development]) }
+      let(:issue1)  { create(:labeled_issue, project: project, labels: [bug, development]) }
+      let(:issue2)  { create(:labeled_issue, project: project, labels: [bug, development]) }
+      let(:params)  { { board_id: board1.id, from_list_id: list1.id, to_list_id: list1.id } }
 
       it 'returns false' do
         expect(described_class.new(project, user, params).execute(issue)).to eq false
@@ -90,6 +92,18 @@ describe Boards::Issues::MoveService, services: true do
 
         expect(issue.reload.labels).to contain_exactly(bug, development)
       end
+
+      it 'sorts issues' do
+        [issue, issue1, issue2].each do |issue|
+          issue.move_to_end && issue.save!
+        end
+
+        params.merge!(move_after_iid: issue1.iid, move_before_iid: issue2.iid)
+
+        described_class.new(project, user, params).execute(issue)
+
+        expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
+      end
     end
   end
 end

spec/services/issues/update_service_spec.rb

@@ -58,6 +58,22 @@ describe Issues::UpdateService, services: true do
         expect(issue.due_date).to eq Date.tomorrow
       end
 
+      it 'sorts issues as specified by parameters' do
+        issue1 = create(:issue, project: project, assignee_id: user3.id)
+        issue2 = create(:issue, project: project, assignee_id: user3.id)
+
+        [issue, issue1, issue2].each do |issue|
+          issue.move_to_end
+          issue.save
+        end
+
+        opts[:move_between_iids] = [issue1.iid, issue2.iid]
+
+        update_issue(opts)
+
+        expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
+      end
+
       context 'when current user cannot admin issues in the project' do
         let(:guest) { create(:user) }
         before do