Use zero-based positions on issues board services

9 files changed, 57 insertions, 48 deletions

diff --git a/app/services/boards/lists/create_service.rb b/app/services/boards/lists/create_service.rb
index 59e8b51e37f..77c3c85df92 100644
--- a/app/services/boards/lists/create_service.rb
+++ b/app/services/boards/lists/create_service.rb
@@ -12,7 +12,8 @@ module Boards
       private
 
       def find_next_position
-        board.lists.label.maximum(:position).to_i + 1
+        max_position = board.lists.label.maximum(:position)
+        max_position.nil? ? 0 : max_position.succ
       end
 
       def create_list_at(position)
diff --git a/app/services/boards/lists/move_service.rb b/app/services/boards/lists/move_service.rb
index 9bd07f43a36..1c91fed0ff4 100644
--- a/app/services/boards/lists/move_service.rb
+++ b/app/services/boards/lists/move_service.rb
@@ -19,7 +19,7 @@ module Boards
 
       def valid_move?
         new_position.present? && new_position != old_position &&
-          new_position >= 0 && new_position <= board.lists.label.size
+          new_position >= 0 && new_position < board.lists.label.size
       end
 
       def old_position
diff --git a/spec/controllers/projects/board_issues_controller_spec.rb b/spec/controllers/projects/board_issues_controller_spec.rb
index c7fccfbce6c..6b4fb382b16 100644
--- a/spec/controllers/projects/board_issues_controller_spec.rb
+++ b/spec/controllers/projects/board_issues_controller_spec.rb
@@ -7,8 +7,8 @@ describe Projects::BoardIssuesController do
   let(:planning)    { create(:label, project: project, name: 'Planning') }
   let(:development) { create(:label, project: project, name: 'Development') }
 
-  let!(:list1) { create(:list, board: project.board, label: planning, position: 1) }
-  let!(:list2) { create(:list, board: project.board, label: development, position: 2) }
+  let!(:list1) { create(:list, board: project.board, label: planning, position: 0) }
+  let!(:list2) { create(:list, board: project.board, label: development, position: 1) }
 
   before do
     project.team << [user, :master]
diff --git a/spec/controllers/projects/board_lists_controller_spec.rb b/spec/controllers/projects/board_lists_controller_spec.rb
index 41d0432e14f..cf8801ca4f6 100644
--- a/spec/controllers/projects/board_lists_controller_spec.rb
+++ b/spec/controllers/projects/board_lists_controller_spec.rb
@@ -47,20 +47,20 @@ describe Projects::BoardListsController do
   end
 
   describe 'PATCH #update' do
-    let!(:planning)    { create(:list, board: board, position: 1) }
-    let!(:development) { create(:list, board: board, position: 2) }
+    let!(:planning)    { create(:list, board: board, position: 0) }
+    let!(:development) { create(:list, board: board, position: 1) }
 
     context 'with valid position' do
       it 'returns a successful 200 response' do
-        move list: planning, position: 2
+        move list: planning, position: 1
 
         expect(response).to have_http_status(200)
       end
 
       it 'moves the list to the desired position' do
-        move list: planning, position: 2
+        move list: planning, position: 1
 
-        expect(planning.reload.position).to eq 2
+        expect(planning.reload.position).to eq 1
       end
     end
 
@@ -74,7 +74,7 @@ describe Projects::BoardListsController do
 
     context 'with invalid list id' do
       it 'returns a not found 404 response' do
-        move list: 999, position: 2
+        move list: 999, position: 1
 
         expect(response).to have_http_status(404)
       end
@@ -91,7 +91,7 @@ describe Projects::BoardListsController do
 
   describe 'DELETE #destroy' do
     context 'with valid list id' do
-      let!(:planning) { create(:list, board: board, position: 1) }
+      let!(:planning) { create(:list, board: board, position: 0) }
 
       it 'returns a successful 200 response' do
         remove_board_list list: planning
@@ -112,7 +112,7 @@ describe Projects::BoardListsController do
       end
     end
 
-    def remove_board_list(list)
+    def remove_board_list(list:)
       delete :destroy, namespace_id: project.namespace.to_param,
                        project_id: project.to_param,
                        id: list.to_param,
diff --git a/spec/services/boards/issues/list_service_spec.rb b/spec/services/boards/issues/list_service_spec.rb
index 010f097802d..5735d0bade6 100644
--- a/spec/services/boards/issues/list_service_spec.rb
+++ b/spec/services/boards/issues/list_service_spec.rb
@@ -14,8 +14,8 @@ describe Boards::Issues::ListService, services: true do
     let(:p3) { create(:label, title: 'P3', project: project, priority: 3) }
 
     let!(:backlog) { create(:backlog_list, board: board) }
-    let!(:list1)   { create(:list, board: board, label: development) }
-    let!(:list2)   { create(:list, board: board, label: testing) }
+    let!(:list1)   { create(:list, board: board, label: development, position: 0) }
+    let!(:list2)   { create(:list, board: board, label: testing, position: 1) }
     let!(:done)    { create(:done_list, board: board) }
 
     let!(:opened_issue1) { create(:labeled_issue, project: project, labels: [bug]) }
diff --git a/spec/services/boards/issues/move_service_spec.rb b/spec/services/boards/issues/move_service_spec.rb
index 285dd61a0e6..9a477b38d47 100644
--- a/spec/services/boards/issues/move_service_spec.rb
+++ b/spec/services/boards/issues/move_service_spec.rb
@@ -11,8 +11,8 @@ describe Boards::Issues::MoveService, services: true do
     let(:testing)  { create(:label, project: project, name: 'Testing') }
 
     let(:backlog) { create(:backlog_list, board: board) }
-    let(:list1)   { create(:list, board: board, label: development) }
-    let(:list2)   { create(:list, board: board, label: testing) }
+    let(:list1)   { create(:list, board: board, label: development, position: 0) }
+    let(:list2)   { create(:list, board: board, label: testing, position: 1) }
     let(:done)    { create(:done_list, board: board) }
 
     before do
diff --git a/spec/services/boards/lists/create_service_spec.rb b/spec/services/boards/lists/create_service_spec.rb
index 27dd4185adb..5e7e145065e 100644
--- a/spec/services/boards/lists/create_service_spec.rb
+++ b/spec/services/boards/lists/create_service_spec.rb
@@ -13,7 +13,7 @@ describe Boards::Lists::CreateService, services: true do
       it 'creates a new list at beginning of the list' do
         list = service.execute
 
-        expect(list.position).to eq 1
+        expect(list.position).to eq 0
       end
     end
 
@@ -23,18 +23,18 @@ describe Boards::Lists::CreateService, services: true do
 
         list = service.execute
 
-        expect(list.position).to eq 1
+        expect(list.position).to eq 0
       end
     end
 
     context 'when board lists has only labels lists' do
       it 'creates a new list at end of the lists' do
+        create(:list, board: board, position: 0)
         create(:list, board: board, position: 1)
-        create(:list, board: board, position: 2)
 
         list = service.execute
 
-        expect(list.position).to eq 3
+        expect(list.position).to eq 2
       end
     end
 
@@ -42,12 +42,12 @@ describe Boards::Lists::CreateService, services: true 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(:list, board: board, position: 1)
+        list1 = create(:list, board: board, position: 0)
 
         list2 = service.execute
 
-        expect(list1.reload.position).to eq 1
-        expect(list2.reload.position).to eq 2
+        expect(list1.reload.position).to eq 0
+        expect(list2.reload.position).to eq 1
       end
     end
   end
diff --git a/spec/services/boards/lists/destroy_service_spec.rb b/spec/services/boards/lists/destroy_service_spec.rb
index 4e382017c00..3b887a55a97 100644
--- a/spec/services/boards/lists/destroy_service_spec.rb
+++ b/spec/services/boards/lists/destroy_service_spec.rb
@@ -16,16 +16,16 @@ describe Boards::Lists::DestroyService, services: true do
 
       it 'decrements position of higher lists' do
         backlog     = create(:backlog_list, board: board)
-        development = create(:list, board: board, position: 1)
-        review      = create(:list, board: board, position: 2)
-        staging     = create(:list, board: board, position: 3)
+        development = create(:list, board: board, position: 0)
+        review      = create(:list, board: board, position: 1)
+        staging     = create(:list, board: board, position: 2)
         done        = create(:done_list, board: board)
 
         described_class.new(project, user, 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(review.reload.position).to eq 0
+        expect(staging.reload.position).to eq 1
         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 aa93cbad7f4..11de74a96a1 100644
--- a/spec/services/boards/lists/move_service_spec.rb
+++ b/spec/services/boards/lists/move_service_spec.rb
@@ -7,10 +7,10 @@ describe Boards::Lists::MoveService, services: true do
     let(:user)    { create(:user) }
 
     let!(:backlog)     { create(:backlog_list, board: board) }
-    let!(:planning)    { create(:list, board: board, position: 1) }
-    let!(:development) { create(:list, board: board, position: 2) }
-    let!(:review)      { create(:list, board: board, position: 3) }
-    let!(:staging)     { create(:list, board: board, position: 4) }
+    let!(:planning)    { create(:list, board: board, position: 0) }
+    let!(:development) { create(:list, board: board, position: 1) }
+    let!(:review)      { create(:list, board: board, position: 2) }
+    let!(:staging)     { create(:list, board: board, position: 3) }
     let!(:done)        { create(:done_list, board: board) }
 
     context 'when list type is set to label' do
@@ -19,15 +19,15 @@ describe Boards::Lists::MoveService, services: true do
 
         service.execute
 
-        expect(current_list_positions).to eq [1, 2, 3, 4]
+        expect(current_list_positions).to eq [0, 1, 2, 3]
       end
 
       it 'keeps position of lists when new positon is equal to old position' do
-        service = described_class.new(project, user, id: planning.id, position: 1)
+        service = described_class.new(project, user, id: planning.id, position: planning.position)
 
         service.execute
 
-        expect(current_list_positions).to eq [1, 2, 3, 4]
+        expect(current_list_positions).to eq [0, 1, 2, 3]
       end
 
       it 'keeps position of lists when new positon is negative' do
@@ -35,47 +35,55 @@ describe Boards::Lists::MoveService, services: true do
 
         service.execute
 
-        expect(current_list_positions).to eq [1, 2, 3, 4]
+        expect(current_list_positions).to eq [0, 1, 2, 3]
+      end
+
+      it 'keeps position of lists when new positon is equal to number of labels lists' do
+        service = described_class.new(project, user, id: planning.id, position: board.lists.label.size)
+
+        service.execute
+
+        expect(current_list_positions).to eq [0, 1, 2, 3]
       end
 
       it 'keeps position of lists when new positon is greater than number of labels lists' do
-        service = described_class.new(project, user, id: planning.id, position: 6)
+        service = described_class.new(project, user, id: planning.id, position: board.lists.label.size + 1)
 
         service.execute
 
-        expect(current_list_positions).to eq [1, 2, 3, 4]
+        expect(current_list_positions).to eq [0, 1, 2, 3]
       end
 
       it 'increments position of intermediate lists when new positon is equal to first position' do
-        service = described_class.new(project, user, id: staging.id, position: 1)
+        service = described_class.new(project, user, id: staging.id, position: 0)
 
         service.execute
 
-        expect(current_list_positions).to eq [2, 3, 4, 1]
+        expect(current_list_positions).to eq [1, 2, 3, 0]
       end
 
       it 'decrements position of intermediate lists when new positon is equal to last position' do
-        service = described_class.new(project, user, id: planning.id, position: 4)
+        service = described_class.new(project, user, id: planning.id, position: board.lists.label.last.position)
 
         service.execute
 
-        expect(current_list_positions).to eq [4, 1, 2, 3]
+        expect(current_list_positions).to eq [3, 0, 1, 2]
       end
 
       it 'decrements position of intermediate lists when new position is greater than old position' do
-        service = described_class.new(project, user, id: planning.id, position: 3)
+        service = described_class.new(project, user, id: planning.id, position: 2)
 
         service.execute
 
-        expect(current_list_positions).to eq [3, 1, 2, 4]
+        expect(current_list_positions).to eq [2, 0, 1, 3]
       end
 
       it 'increments position of intermediate lists when new position is lower than old position' do
-        service = described_class.new(project, user, id: staging.id, position: 2)
+        service = described_class.new(project, user, id: staging.id, position: 1)
 
         service.execute
 
-        expect(current_list_positions).to eq [1, 3, 4, 2]
+        expect(current_list_positions).to eq [0, 2, 3, 1]
       end
     end
 
@@ -84,7 +92,7 @@ describe Boards::Lists::MoveService, services: true do
 
       service.execute
 
-      expect(current_list_positions).to eq [1, 2, 3, 4]
+      expect(current_list_positions).to eq [0, 1, 2, 3]
     end
 
     it 'keeps position of lists when list type is done' do
@@ -92,7 +100,7 @@ describe Boards::Lists::MoveService, services: true do
 
       service.execute
 
-      expect(current_list_positions).to eq [1, 2, 3, 4]
+      expect(current_list_positions).to eq [0, 1, 2, 3]
     end
   end