From 9e5a305f18377c9fbe74e44dcef7606ab109291c Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 3 Aug 2016 12:09:03 -0300 Subject: [PATCH] Use zero-based positions on issues board services --- app/services/boards/lists/create_service.rb | 3 +- app/services/boards/lists/move_service.rb | 2 +- .../projects/board_issues_controller_spec.rb | 4 +- .../projects/board_lists_controller_spec.rb | 16 ++-- .../boards/issues/list_service_spec.rb | 4 +- .../boards/issues/move_service_spec.rb | 4 +- .../boards/lists/create_service_spec.rb | 14 ++-- .../boards/lists/destroy_service_spec.rb | 10 +-- .../boards/lists/move_service_spec.rb | 84 ++++++++++--------- 9 files changed, 75 insertions(+), 66 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 greater than number of labels lists' do - service = described_class.new(project, user, id: planning.id, position: 6) + 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 [1, 2, 3, 4] + 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: board.lists.label.size + 1) + + service.execute + + 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: 0) + + service.execute + + 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: board.lists.label.last.position) + + service.execute + + 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: 2) + + service.execute + + 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: 1) service.execute - expect(current_list_positions).to eq [2, 3, 4, 1] - 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.execute - - 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, user, id: planning.id, position: 3) - - service.execute - - expect(current_list_positions).to eq [3, 1, 2, 4] - 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.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