Allow issues in boards to be ordered

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

@@ -86,7 +86,7 @@ require('./board_new_issue');
       const options = gl.issueBoards.getBoardSortableDefaultOptions({
         scroll: document.querySelectorAll('.boards-list')[0],
         group: 'issues',
-        sort: false,
+        sort: true,
         disabled: this.disabled,
         filter: '.board-list-count, .is-disabled',
         onStart: (e) => {
@@ -105,6 +105,9 @@ require('./board_new_issue');
             e.item.remove();
           });
         },
+        onUpdate: (e) => {
+          gl.issueBoards.BoardsStore.moveIssueInList(this.list, Store.moving.issue, e.oldIndex, e.newIndex);
+        },
       });
 
       this.sortable = Sortable.create(this.$refs.list, options);

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

@@ -111,8 +111,16 @@ class List {
 
   addIssue (issue, listFrom, newIndex) {
     if (!this.findIssue(issue.id)) {
+      let moveBeforeIid = null;
+      let moveAfterIid = null;
+
       if (newIndex !== undefined) {
         this.issues.splice(newIndex, 0, issue);
+
+        const issueBefore = this.issues[newIndex - 1];
+        if (issueBefore) moveAfterIid = issueBefore.id;
+        const issueAfter = this.issues[newIndex + 1];
+        if (issueAfter) moveBeforeIid = issueAfter.id;
       } else {
         this.issues.push(issue);
       }
@@ -123,7 +131,7 @@ class List {
 
       if (listFrom) {
         this.issuesSize += 1;
-        gl.boardService.moveIssue(issue.id, listFrom.id, this.id)
+        gl.boardService.moveIssue(issue.id, listFrom.id, this.id, moveAfterIid, moveBeforeIid)
           .then(() => {
             listFrom.getIssues(false);
           });
@@ -131,6 +139,21 @@ class List {
     }
   }
 
+  moveIssue (issue, oldIndex, newIndex) {
+    let moveBeforeIid = null;
+    let moveAfterIid = null;
+
+    this.issues.splice(oldIndex, 1);
+    this.issues.splice(newIndex, 0, issue);
+
+    const issueBefore = this.issues[newIndex - 1];
+    if (issueBefore) moveAfterIid = issueBefore.id;
+    const issueAfter = this.issues[newIndex + 1];
+    if (issueAfter) moveBeforeIid = issueAfter.id;
+
+    gl.boardService.moveIssue(issue.id, null, null, moveAfterIid, moveBeforeIid);
+  }
+
   findIssue (id) {
     return this.issues.filter(issue => issue.id === id)[0];
   }

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

@@ -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_after_iid = null, move_before_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.es6

@@ -106,6 +106,9 @@
         listFrom.removeIssue(issue);
       }
     },
+    moveIssueInList (list, issue, oldIndex, newIndex) {
+      list.moveIssue(issue, oldIndex, newIndex);
+    },
     findList (key, val, type = 'label') {
       return this.state.lists.filter((list) => {
         const byType = type ? list['type'] === type : true;

app/controllers/projects/boards/issues_controller.rb

@@ -63,7 +63,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

app/models/concerns/relative_positioning.rb

@@ -0,0 +1,132 @@
+module RelativePositioning
+  extend ActiveSupport::Concern
+
+  MIN_POSITION = Float::MIN
+  MAX_POSITION = Float::MAX
+
+  included do
+    after_save :save_positionable_neighbours
+  end
+
+  def min_relative_position
+    self.class.minimum(:relative_position)
+  end
+
+  def max_relative_position
+    self.class.maximum(:relative_position)
+  end
+
+  def prev_relative_position
+    prev_pos = nil
+
+    if self.relative_position
+      prev_pos = self.class.
+        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.
+        where('relative_position > ?', self.relative_position).
+        minimum(:relative_position)
+    end
+
+    next_pos || MAX_POSITION
+  end
+
+  def move_between(before, after)
+    return move_nowhere unless before || after
+    return move_after(before) if before && !after
+    return move_before(after) if after && !before
+
+    pos_before = before.relative_position
+    pos_after = after.relative_position
+
+    if pos_before && pos_after
+      if pos_before == pos_after
+        pos = pos_before
+
+        self.relative_position = pos
+        before.move_before(self)
+        after.move_after(self)
+        @positionable_neighbours = [before, after]
+      else
+        self.relative_position = position_between(pos_before, pos_after)
+      end
+    elsif pos_before
+      self.move_after(before)
+      after.move_after(self)
+      @positionable_neighbours = [after]
+    elsif pos_after
+      self.move_before(after)
+      before.move_before(self)
+      @positionable_neighbours = [before]
+    else
+      move_to_end
+      before.move_before(self)
+      after.move_after(self)
+      @positionable_neighbours = [before, after]
+    end
+  end
+
+  def move_before(after)
+    pos_after = after.relative_position
+    if pos_after
+      self.relative_position = position_between(after.prev_relative_position, pos_after)
+    else
+      move_to_end
+      after.move_after(self)
+      @positionable_neighbours = [after]
+    end
+  end
+
+  def move_after(before)
+    pos_before = before.relative_position
+    if pos_before
+      self.relative_position = position_between(pos_before, before.next_relative_position)
+    else
+      move_to_end
+      before.move_before(self)
+      @positionable_neighbours = [before]
+    end
+  end
+
+  def move_nowhere
+    self.relative_position = nil
+  end
+
+  def move_to_front
+    self.relative_position = position_between(MIN_POSITION, min_relative_position || MAX_POSITION)
+  end
+
+  def move_to_end
+    self.relative_position = position_between(max_relative_position || MIN_POSITION, MAX_POSITION)
+  end
+
+  def move_between!(*args)
+    move_between(*args) && save!
+  end
+
+  private
+
+  def position_between(pos_before, pos_after)
+    pos_before, pos_after = [pos_before, pos_after].sort
+
+    pos_before + (pos_after - pos_before) / 2
+  end
+
+  def save_positionable_neighbours
+    return unless @positionable_neighbours
+
+    @positionable_neighbours.each(&:save)
+    @positionable_neighbours = nil
+
+    true
+  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,13 @@ module Boards
 
         Array(label_ids).compact
       end
+
+      def move_between_iids
+        move_after_iid = params[:move_after_iid]
+        move_before_iid = params[:move_before_iid]
+        return unless move_after_iid || move_before_iid
+        [move_after_iid, move_before_iid]
+      end
     end
   end
 end

app/services/issuable_base_service.rb

@@ -212,7 +212,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? && update_issuable(issuable, params)
+    if (issuable.changed? || params.present?) && update_issuable(issuable, params)
       # We do not touch as it will affect a update on updated_at field
       ActiveRecord::Base.no_touching do
         handle_common_system_notes(issuable, old_labels: old_labels)

app/services/issues/update_service.rb

@@ -1,6 +1,7 @@
 module Issues
   class UpdateService < Issues::BaseService
     def execute(issue)
+      handle_move_between_iids(issue)
       update(issue)
     end
 
@@ -47,6 +48,26 @@ module Issues
       Issues::CloseService
     end
 
+    def handle_move_between_iids(issue)
+      if move_between_iids = params.delete(:move_between_iids)
+        before_iid, after_iid = move_between_iids
+
+        issue_before = nil
+        if before_iid
+          issue_before = issue.project.issues.find_by(iid: before_iid)
+          issue_before = nil unless can?(current_user, :update_issue, issue_before)
+        end
+
+        issue_after = nil
+        if after_iid
+          issue_after = issue.project.issues.find_by(iid: after_iid)
+          issue_after = nil unless can?(current_user, :update_issue, issue_after)
+        end
+
+        issue.move_between(issue_before, issue_after)
+      end
+    end
+
     private
 
     def create_confidentiality_note(issue)

db/migrate/20170131221752_add_relative_position_to_issues.rb

@@ -0,0 +1,31 @@
+# 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 change
+    add_column :issues, :relative_position, :float
+
+    add_index :issues, :relative_position
+  end
+end

db/schema.rb

@@ -87,9 +87,9 @@ ActiveRecord::Schema.define(version: 20170214111112) do
     t.boolean "send_user_confirmation_email", default: false
     t.integer "container_registry_token_expire_delay", default: 5
     t.text "after_sign_up_text"
-    t.boolean "user_default_external", default: false, null: false
     t.string "repository_storages", default: "default"
     t.string "enabled_git_access_protocol"
+    t.boolean "user_default_external", default: false, null: false
     t.boolean "domain_blacklist_enabled", default: false
     t.text "domain_blacklist"
     t.boolean "koding_enabled"
@@ -402,22 +402,22 @@ ActiveRecord::Schema.define(version: 20170214111112) do
   add_index "deploy_keys_projects", ["project_id"], name: "index_deploy_keys_projects_on_project_id", using: :btree
 
   create_table "deployments", force: :cascade do |t|
-    t.integer "iid", null: false
-    t.integer "project_id", null: false
-    t.integer "environment_id", null: false
-    t.string "ref", null: false
-    t.boolean "tag", null: false
-    t.string "sha", null: false
+    t.integer "iid"
+    t.integer "project_id"
+    t.integer "environment_id"
+    t.string "ref"
+    t.boolean "tag"
+    t.string "sha"
     t.integer "user_id"
-    t.integer "deployable_id"
-    t.string "deployable_type"
+    t.integer "deployable_id", null: false
+    t.string "deployable_type", null: false
     t.datetime "created_at"
     t.datetime "updated_at"
     t.string "on_stop"
   end
 
   add_index "deployments", ["project_id", "environment_id", "iid"], name: "index_deployments_on_project_id_and_environment_id_and_iid", using: :btree
-  add_index "deployments", ["project_id", "iid"], name: "index_deployments_on_project_id_and_iid", unique: true, using: :btree
+  add_index "deployments", ["project_id", "iid"], name: "index_deployments_on_project_id_and_iid", using: :btree
 
   create_table "emails", force: :cascade do |t|
     t.integer "user_id", null: false
@@ -514,6 +514,7 @@ ActiveRecord::Schema.define(version: 20170214111112) do
     t.text "title_html"
     t.text "description_html"
     t.integer "time_estimate"
+    t.float "relative_position"
   end
 
   add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
@@ -525,6 +526,7 @@ ActiveRecord::Schema.define(version: 20170214111112) 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"}
 
@@ -692,8 +694,8 @@ ActiveRecord::Schema.define(version: 20170214111112) do
     t.integer "merge_user_id"
     t.string "merge_commit_sha"
     t.datetime "deleted_at"
-    t.string "in_progress_merge_commit_sha"
     t.integer "lock_version"
+    t.string "in_progress_merge_commit_sha"
     t.text "title_html"
     t.text "description_html"
     t.integer "time_estimate"
@@ -770,6 +772,16 @@ ActiveRecord::Schema.define(version: 20170214111112) do
   add_index "namespaces", ["path"], name: "index_namespaces_on_path_trigram", using: :gin, opclasses: {"path"=>"gin_trgm_ops"}
   add_index "namespaces", ["type"], name: "index_namespaces_on_type", using: :btree
 
+  create_table "note_templates", force: :cascade do |t|
+    t.integer "user_id"
+    t.string "title"
+    t.text "note"
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+  end
+
+  add_index "note_templates", ["user_id"], name: "index_note_templates_on_user_id", using: :btree
+
   create_table "notes", force: :cascade do |t|
     t.text "note"
     t.string "noteable_type"
@@ -785,6 +797,7 @@ ActiveRecord::Schema.define(version: 20170214111112) do
     t.text "st_diff"
     t.integer "updated_by_id"
     t.string "type"
+    t.string "system_type"
     t.text "position"
     t.text "original_position"
     t.datetime "resolved_at"
@@ -978,7 +991,7 @@ ActiveRecord::Schema.define(version: 20170214111112) do
     t.boolean "has_external_wiki"
     t.boolean "lfs_enabled"
     t.text "description_html"
-    t.boolean "only_allow_merge_if_all_discussions_are_resolved"
+    t.boolean "only_allow_merge_if_all_discussions_are_resolved", default: false, null: false
   end
 
   add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree