Order board lists by list_type, and position

14 files changed, 113 insertions, 103 deletions

diff --git a/app/models/board.rb b/app/models/board.rb
index d44df497e78..d6358fb15e8 100644
--- a/app/models/board.rb
+++ b/app/models/board.rb
@@ -1,7 +1,7 @@
 class Board < ActiveRecord::Base
   belongs_to :project
 
-  has_many :lists, dependent: :destroy
+  has_many :lists, -> { order(:list_type, :position) }, dependent: :destroy
 
   validates :project, presence: true
 end
diff --git a/app/models/list.rb b/app/models/list.rb
index 7b347c2255b..38cf2050527 100644
--- a/app/models/list.rb
+++ b/app/models/list.rb
@@ -2,9 +2,9 @@ class List < ActiveRecord::Base
   belongs_to :board
   belongs_to :label
 
-  enum list_type: { label: 0, backlog: 1, done: 2 }
+  enum list_type: { backlog: 0, label: 1, done: 2 }
 
-  validates :board, :list_type, :position, presence: true
-  validates :label, presence: true, if: :label?
-  validates :position, numericality: { only_integer: true, greater_than_or_equal_to: 0 }
+  validates :board, :list_type, presence: true
+  validates :label, :position, presence: true, if: :label?
+  validates :position, numericality: { only_integer: true, greater_than_or_equal_to: 0 }, if: :label?
 end
diff --git a/app/services/boards/create_service.rb b/app/services/boards/create_service.rb
index 44e937af8c6..f1eef870925 100644
--- a/app/services/boards/create_service.rb
+++ b/app/services/boards/create_service.rb
@@ -15,8 +15,8 @@ module Boards
 
     def create_board!
       project.create_board
-      project.board.lists.create(list_type: :backlog, position: 0)
-      project.board.lists.create(list_type: :done, position: 1)
+      project.board.lists.create(list_type: :backlog)
+      project.board.lists.create(list_type: :done)
     end
   end
 end
diff --git a/app/services/boards/lists/create_service.rb b/app/services/boards/lists/create_service.rb
index 42ab93ad8c4..151bb67bea1 100644
--- a/app/services/boards/lists/create_service.rb
+++ b/app/services/boards/lists/create_service.rb
@@ -19,10 +19,7 @@ module Boards
       attr_reader :board, :params
 
       def find_next_position
-        return 0 unless board.lists.any?
-
-        records = board.lists.where.not(list_type: List.list_types[:done])
-        records.maximum(:position).to_i + 1
+        board.lists.label.maximum(:position).to_i + 1
       end
 
       def create_list_at(position)
@@ -30,8 +27,8 @@ module Boards
       end
 
       def increment_higher_lists(position)
-        board.lists.where('position >= ?', position)
-             .update_all('position = position + 1')
+        board.lists.label.where('position >= ?', position)
+                         .update_all('position = position + 1')
       end
     end
   end
diff --git a/app/services/boards/lists/destroy_service.rb b/app/services/boards/lists/destroy_service.rb
index ba097942aa6..4e0b3012775 100644
--- a/app/services/boards/lists/destroy_service.rb
+++ b/app/services/boards/lists/destroy_service.rb
@@ -10,7 +10,7 @@ module Boards
         return false unless list.label?
 
         list.with_lock do
-          reorder_higher_lists
+          decrement_higher_lists
           remove_list
         end
       end
@@ -23,9 +23,9 @@ module Boards
         @list ||= board.lists.find(params[:list_id])
       end
 
-      def reorder_higher_lists
-        board.lists.where('position > ?',  list.position)
-             .update_all('position = position - 1')
+      def decrement_higher_lists
+        board.lists.label.where('position > ?',  list.position)
+                   .update_all('position = position - 1')
       end
 
       def remove_list
diff --git a/app/services/boards/lists/move_service.rb b/app/services/boards/lists/move_service.rb
index f6705b5afce..0f2cb7f4b8b 100644
--- a/app/services/boards/lists/move_service.rb
+++ b/app/services/boards/lists/move_service.rb
@@ -8,7 +8,7 @@ module Boards
 
       def execute
         return false unless list.label?
-        return false if invalid_position?
+        return false unless valid_move?
 
         list.with_lock do
           reorder_intermediate_lists
@@ -24,18 +24,9 @@ module Boards
         @list ||= board.lists.find(params[:list_id])
       end
 
-      def invalid_position?
-        return true if new_position.blank?
-
-        [old_position, first_position, last_position].include?(new_position)
-      end
-
-      def first_position
-        board.lists.first.try(:position)
-      end
-
-      def last_position
-        board.lists.last.try(:position)
+      def valid_move?
+        new_position.present? && new_position != old_position &&
+          new_position >= 0 && new_position <= board.lists.label.size
       end
 
       def old_position
@@ -55,15 +46,15 @@ module Boards
       end
 
       def decrement_intermediate_lists
-        board.lists.where('position > ?',  old_position)
-             .where('position <= ?', new_position)
-             .update_all('position = position - 1')
+        board.lists.label.where('position > ?',  old_position)
+                         .where('position <= ?', new_position)
+                         .update_all('position = position - 1')
       end
 
       def increment_intermediate_lists
-        board.lists.where('position >= ?', new_position)
-                   .where('position < ?',  old_position)
-                   .update_all('position = position + 1')
+        board.lists.label.where('position >= ?', new_position)
+                         .where('position < ?',  old_position)
+                         .update_all('position = position + 1')
       end
 
       def update_list_position
diff --git a/db/migrate/20160727193336_create_lists.rb b/db/migrate/20160727193336_create_lists.rb
index 12bb5605aad..61d501215f2 100644
--- a/db/migrate/20160727193336_create_lists.rb
+++ b/db/migrate/20160727193336_create_lists.rb
@@ -7,8 +7,8 @@ class CreateLists < ActiveRecord::Migration
     create_table :lists do |t|
       t.references :board, index: true, foreign_key: true, null: false
       t.references :label, index: true, foreign_key: true
-      t.integer :list_type, null: false, default: 0
-      t.integer :position, null: false
+      t.integer :list_type, null: false, default: 1
+      t.integer :position
 
       t.timestamps null: false
     end
diff --git a/db/schema.rb b/db/schema.rb
index aec00136ff8..873dfa81702 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -544,8 +544,8 @@ ActiveRecord::Schema.define(version: 20160810142633) do
   create_table "lists", force: :cascade do |t|
     t.integer  "board_id",               null: false
     t.integer  "label_id"
-    t.integer  "list_type",  default: 0, null: false
-    t.integer  "position",               null: false
+    t.integer  "list_type",  default: 1, null: false
+    t.integer  "position"
     t.datetime "created_at",             null: false
     t.datetime "updated_at",             null: false
   end
diff --git a/spec/factories/lists.rb b/spec/factories/lists.rb
index bef161d2a7e..7b41361500c 100644
--- a/spec/factories/lists.rb
+++ b/spec/factories/lists.rb
@@ -2,18 +2,22 @@ FactoryGirl.define do
   factory :list do
     board
     label
-    sequence(:position)
   end
 
   factory :backlog_list, parent: :list do
     list_type :backlog
+    label nil
+    position nil
   end
 
   factory :label_list, parent: :list do
     list_type :label
+    sequence(:position)
   end
 
   factory :done_list, parent: :list do
     list_type :done
+    label nil
+    position nil
   end
 end
diff --git a/spec/models/board_spec.rb b/spec/models/board_spec.rb
index 1f32a84c340..23a91619a27 100644
--- a/spec/models/board_spec.rb
+++ b/spec/models/board_spec.rb
@@ -3,7 +3,7 @@ require 'rails_helper'
 describe Board do
   describe 'relationships' do
     it { is_expected.to belong_to(:project) }
-    it { is_expected.to have_many(:lists).dependent(:destroy) }
+    it { is_expected.to have_many(:lists).order(list_type: :asc, position: :asc).dependent(:destroy) }
   end
 
   describe 'validations' do
diff --git a/spec/models/list_spec.rb b/spec/models/list_spec.rb
index 7ad32047cb7..60b46f06195 100644
--- a/spec/models/list_spec.rb
+++ b/spec/models/list_spec.rb
@@ -13,16 +13,18 @@ describe List do
     it { is_expected.to validate_presence_of(:position) }
     it { is_expected.to validate_numericality_of(:position).only_integer.is_greater_than_or_equal_to(0) }
 
-    it 'does not require label to be set when list_type is set to backlog' do
-      subject.list_type = :backlog
+    context 'when list_type is set to backlog' do
+      subject { described_class.new(list_type: :backlog) }
 
-      expect(subject).not_to validate_presence_of(:label)
+      it { is_expected.not_to validate_presence_of(:label) }
+      it { is_expected.not_to validate_presence_of(:position) }
     end
 
-    it 'does not require label to be set when list_type is set to done' do
-      subject.list_type = :done
+    context 'when list_type is set to done' do
+      subject { described_class.new(list_type: :done) }
 
-      expect(subject).not_to validate_presence_of(:label)
+      it { is_expected.not_to validate_presence_of(:label) }
+      it { is_expected.not_to validate_presence_of(:position) }
     end
   end
 end
diff --git a/spec/services/boards/lists/create_service_spec.rb b/spec/services/boards/lists/create_service_spec.rb
index 09cef364a3b..0e8190794a0 100644
--- a/spec/services/boards/lists/create_service_spec.rb
+++ b/spec/services/boards/lists/create_service_spec.rb
@@ -9,16 +9,16 @@ describe Boards::Lists::CreateService, services: true do
     subject(:service) { described_class.new(project, label_id: label.id) }
 
     context 'when board lists is empty' do
-      it 'creates a new list at begginning of the list' do
+      it 'creates a new list at beginning of the list' do
         list = service.execute
 
-        expect(list.position).to eq 0
+        expect(list.position).to eq 1
       end
     end
 
-    context 'when board lists has a backlog list' do
-      it 'creates a new list at end of the list' do
-        create(:backlog_list, board: board, position: 0)
+    context 'when board lists has only a backlog list' do
+      it 'creates a new list at beginning of the list' do
+        create(:backlog_list, board: board)
 
         list = service.execute
 
@@ -26,9 +26,10 @@ describe Boards::Lists::CreateService, services: true do
       end
     end
 
-    context 'when board lists are only labels lists' do
-      it 'creates a new list at end of the list' do
-        create_list(:label_list, 2, board: board)
+    context 'when board lists has only labels lists' do
+      it 'creates a new list at end of the lists' do
+        create(:label_list, board: board, position: 1)
+        create(:label_list, board: board, position: 2)
 
         list = described_class.new(project, label_id: label.id).execute
 
@@ -36,18 +37,16 @@ describe Boards::Lists::CreateService, services: true do
       end
     end
 
-    context 'when board lists has a done list' do
-      it 'creates a new list before' do
-        list1 = create(:backlog_list, board: board, position: 1)
-        list2 = create(:label_list, board: board, position: 2)
-        list3 = create(:done_list, board: board, position: 3)
+    context 'when board lists has backlog, label and done lists' do
+      it 'creates a new list at end of the label lists' do
+        create(:backlog_list, board: board)
+        create(:done_list, board: board)
+        list1 = create(:label_list, board: board, position: 1)
 
-        list = described_class.new(project, label_id: label.id).execute
+        list2 = described_class.new(project, label_id: label.id).execute
 
-        expect(list.position).to eq 3
         expect(list1.reload.position).to eq 1
         expect(list2.reload.position).to eq 2
-        expect(list3.reload.position).to eq 4
       end
     end
   end
diff --git a/spec/services/boards/lists/destroy_service_spec.rb b/spec/services/boards/lists/destroy_service_spec.rb
index 85cc3a0bc67..4d50bd2d123 100644
--- a/spec/services/boards/lists/destroy_service_spec.rb
+++ b/spec/services/boards/lists/destroy_service_spec.rb
@@ -14,18 +14,18 @@ describe Boards::Lists::DestroyService, services: true do
       end
 
       it 'decrements position of higher lists' do
-        list1 = create(:backlog_list, board: board, position: 1)
-        list2 = create(:label_list, board: board, position: 2)
-        list3 = create(:label_list, board: board, position: 3)
-        list4 = create(:label_list, board: board, position: 4)
-        list5 = create(:done_list, board: board, position: 5)
-
-        described_class.new(project, list_id: list2.id).execute
-
-        expect(list1.reload.position).to eq 1
-        expect(list3.reload.position).to eq 2
-        expect(list4.reload.position).to eq 3
-        expect(list5.reload.position).to eq 4
+        backlog     = create(:backlog_list, board: board)
+        development = create(:label_list, board: board, position: 1)
+        review      = create(:label_list, board: board, position: 2)
+        staging     = create(:label_list, board: board, position: 3)
+        done        = create(:done_list, board: board)
+
+        described_class.new(project, list_id: development.id).execute
+
+        expect(backlog.reload.position).to be_nil
+        expect(review.reload.position).to eq 1
+        expect(staging.reload.position).to eq 2
+        expect(done.reload.position).to be_nil
       end
     end
 
diff --git a/spec/services/boards/lists/move_service_spec.rb b/spec/services/boards/lists/move_service_spec.rb
index 072087a0e05..97de502e069 100644
--- a/spec/services/boards/lists/move_service_spec.rb
+++ b/spec/services/boards/lists/move_service_spec.rb
@@ -4,81 +4,98 @@ describe Boards::Lists::MoveService, services: true do
   describe '#execute' do
     let(:project) { create(:project_with_board) }
     let(:board)   { project.board }
-    let!(:list1)  { create(:backlog_list, board: board, position: 1) }
-    let!(:list2)  { create(:label_list,   board: board, position: 2) }
-    let!(:list3)  { create(:label_list,   board: board, position: 3) }
-    let!(:list4)  { create(:label_list,   board: board, position: 4) }
-    let!(:list5)  { create(:label_list,   board: board, position: 5) }
-    let!(:list6)  { create(:done_list,    board: board, position: 6) }
+
+    let!(:backlog)     { create(:backlog_list, board: board) }
+    let!(:planning)    { create(:label_list,   board: board, position: 1) }
+    let!(:development) { create(:label_list,   board: board, position: 2) }
+    let!(:review)      { create(:label_list,   board: board, position: 3) }
+    let!(:staging)     { create(:label_list,   board: board, position: 4) }
+    let!(:done)        { create(:done_list,    board: board) }
 
     context 'when list type is set to label' do
       it 'keeps position of lists when new position is nil' do
-        service = described_class.new(project, { list_id: list2.id, position: nil })
+        service = described_class.new(project, { list_id: planning.id, position: nil })
 
         service.execute
 
-        expect(positions_of_lists).to eq [1, 2, 3, 4, 5, 6]
+        expect(current_list_positions).to eq [1, 2, 3, 4]
       end
 
       it 'keeps position of lists when new positon is equal to old position' do
-        service = described_class.new(project, { list_id: list2.id, position: 2 })
+        service = described_class.new(project, { list_id: planning.id, position: 1 })
+
+        service.execute
+
+        expect(current_list_positions).to eq [1, 2, 3, 4]
+      end
+
+      it 'keeps position of lists when new positon is negative' do
+        service = described_class.new(project, { list_id: planning.id, position: -1 })
+
+        service.execute
+
+        expect(current_list_positions).to eq [1, 2, 3, 4]
+      end
+
+      it 'keeps position of lists when new positon is greater than number of labels lists' do
+        service = described_class.new(project, { list_id: planning.id, position: 6 })
 
         service.execute
 
-        expect(positions_of_lists).to eq [1, 2, 3, 4, 5, 6]
+        expect(current_list_positions).to eq [1, 2, 3, 4]
       end
 
-      it 'keeps position of lists when new positon is equal to first position' do
-        service = described_class.new(project, { list_id: list3.id, position: 1 })
+      it 'increments position of intermediate lists when new positon is equal to first position' do
+        service = described_class.new(project, { list_id: staging.id, position: 1 })
 
         service.execute
 
-        expect(positions_of_lists).to eq [1, 2, 3, 4, 5, 6]
+        expect(current_list_positions).to eq [2, 3, 4, 1]
       end
 
-      it 'keeps position of lists when new positon is equal to last position' do
-        service = described_class.new(project, { list_id: list3.id, position: 6 })
+      it 'decrements position of intermediate lists when new positon is equal to last position' do
+        service = described_class.new(project, { list_id: planning.id, position: 4 })
 
         service.execute
 
-        expect(positions_of_lists).to eq [1, 2, 3, 4, 5, 6]
+        expect(current_list_positions).to eq [4, 1, 2, 3]
       end
 
       it 'decrements position of intermediate lists when new position is greater than old position' do
-        service = described_class.new(project, { list_id: list2.id, position: 5 })
+        service = described_class.new(project, { list_id: planning.id, position: 3 })
 
         service.execute
 
-        expect(positions_of_lists).to eq [1, 5, 2, 3, 4, 6]
+        expect(current_list_positions).to eq [3, 1, 2, 4]
       end
 
-      it 'increments position of intermediate lists when when new position is lower than old position' do
-        service = described_class.new(project, { list_id: list5.id, position: 2 })
+      it 'increments position of intermediate lists when new position is lower than old position' do
+        service = described_class.new(project, { list_id: staging.id, position: 2 })
 
         service.execute
 
-        expect(positions_of_lists).to eq [1, 3, 4, 5, 2, 6]
+        expect(current_list_positions).to eq [1, 3, 4, 2]
       end
     end
 
     it 'keeps position of lists when list type is backlog' do
-      service = described_class.new(project, { list_id: list1.id, position: 2 })
+      service = described_class.new(project, { list_id: backlog.id, position: 2 })
 
       service.execute
 
-      expect(positions_of_lists).to eq [1, 2, 3, 4, 5, 6]
+      expect(current_list_positions).to eq [1, 2, 3, 4]
     end
 
     it 'keeps position of lists when list type is done' do
-      service = described_class.new(project, { list_id: list6.id, position: 2 })
+      service = described_class.new(project, { list_id: done.id, position: 2 })
 
       service.execute
 
-      expect(positions_of_lists).to eq [1, 2, 3, 4, 5, 6]
+      expect(current_list_positions).to eq [1, 2, 3, 4]
     end
   end
 
-  def positions_of_lists
-    (1..6).map { |index| send("list#{index}").reload.position }
+  def current_list_positions
+    [planning, development, review, staging].map { |list| list.reload.position }
   end
 end