Add minor fixes

* make sure that last_for_board_id never returns nil value
* pass correct group id to the position service
* add test for moving a subepic
* fix getting proper relative position for selected board

ee/app/models/boards/epic_board_position.rb

@@ -25,11 +25,11 @@ module Boards
       end
 
       def last_for_board_id(board_id)
-        where(epic_board_id: board_id).order(::Gitlab::Database.nulls_first_order('boards_epic_board_positions.relative_position', 'DESC')).first
+        where(epic_board_id: board_id).where.not(relative_position: nil).order(relative_position: :desc).first
       end
 
       def bulk_upsert(positions)
-        bulk_upsert!(positions, unique_by: %i[epic_board_id epic_id])
+        bulk_upsert!(positions, unique_by: %i[epic_board_id epic_id], validate: false)
       end
     end
   end

ee/app/models/ee/epic.rb

@@ -118,15 +118,23 @@ module EE
         reorder('relative_position ASC', 'id DESC')
       end
 
+      scope :join_board_position, ->(board_id) do
+        epics = ::Epic.arel_table
+        positions = ::Boards::EpicBoardPosition.arel_table
+
+        epic_positions = epics.join(positions, Arel::Nodes::OuterJoin)
+          .on(epics[:id].eq(positions[:epic_id]).and(positions[:epic_board_id].eq(board_id)))
+
+        joins(epic_positions.join_sources)
+      end
+
       scope :order_relative_position_on_board, ->(board_id) do
-        left_joins(:epic_board_positions)
-          .where(boards_epic_board_positions: { epic_board_id: [nil, board_id] })
+        join_board_position(board_id)
           .reorder(::Gitlab::Database.nulls_last_order('boards_epic_board_positions.relative_position', 'ASC'), 'epics.id DESC')
       end
 
-      scope :without_board_position, -> do
-        left_joins(:epic_board_positions)
-          .where(boards_epic_board_positions: { relative_position: nil })
+      scope :without_board_position, ->(board_id) do
+        where(boards_epic_board_positions: { relative_position: nil })
       end
 
       scope :with_api_entity_associations, -> { preload(:author, :labels, group: :route) }

ee/app/services/boards/epics/move_service.rb

@@ -22,7 +22,7 @@ module Boards
 
       override :reposition_params
       def reposition_params(reposition_ids)
-        super.merge(list_id: params[:to_list_id])
+        super.merge(list_id: params[:to_list_id], board_group: parent)
       end
 
       def reposition_parent

ee/app/services/boards/epics/position_create_service.rb

@@ -42,11 +42,15 @@ module Boards
       end
 
       def epics_on_board_list
-        # the positions will be created for all epics with id >= from_id
+        # the positions will be created for all epics in the board list
+        # which don't have position set yet and which appear in the list
+        # before the epic being positioned. Epics w/o position are ordered
+        # by ID in descending order so we need to set position for epics with
+        # id >= from_id
         list_params = { board_id: board_id, id: list_id, from_id: params[:from_id] }
 
         Boards::Epics::ListService.new(parent, current_user, list_params).execute
-          .without_board_position
+          .without_board_position(board_id)
           .select(:id)
           .limit(LIMIT)
       end

ee/app/services/epics/update_service.rb

@@ -105,10 +105,11 @@ module Epics
 
     def reposition_on_board(epic)
       return unless params[:move_between_ids]
-      return unless params[positioning_scope_key]
+      return unless epic_board_id
 
-      fill_missing_positions_before(epic)
-      epic_board_position = issuable_for_positioning(epic.id, params[positioning_scope_key], create_missing: true)
+      fill_missing_positions_before
+
+      epic_board_position = issuable_for_positioning(epic.id, epic_board_id, create_missing: true)
       handle_move_between_ids(epic_board_position)
 
       epic_board_position.save!
@@ -126,20 +127,26 @@ module Epics
       Boards::EpicBoardPosition.create!(epic_id: id, epic_board_id: board_id) if create_missing
     end
 
-    def fill_missing_positions_before(epic)
-      before_id = params[:move_between_ids].second
+    def fill_missing_positions_before
+      before_id = params[:move_between_ids].compact.max
       list_id = params.delete(:list_id)
+      board_group = params.delete(:board_group)
 
-      # if position for the epic above exist we don't need to create positioning records
-      return if issuable_for_positioning(before_id, params[positioning_scope_key])
+      return unless before_id
+      # if position for the epic above exists we don't need to create positioning records
+      return if issuable_for_positioning(before_id, epic_board_id)
 
       service_params = {
-        board_id: params[positioning_scope_key],
+        board_id: epic_board_id,
         list_id: list_id, # we need to have positions only for the current list
         from_id: before_id # we need to have positions only for the epics above
       }
 
-      Boards::Epics::PositionCreateService.new(epic.group, current_user, service_params).execute
+      Boards::Epics::PositionCreateService.new(board_group, current_user, service_params).execute
+    end
+
+    def epic_board_id
+      params[positioning_scope_key]
     end
 
     def positioning_scope_key

ee/spec/graphql/mutations/boards/epic_boards/epic_move_list_spec.rb

@@ -46,7 +46,7 @@ RSpec.describe ::Mutations::Boards::EpicBoards::EpicMoveList do
     end
 
     context 'when user does not have permissions' do
-      it 'moves the epic to another list' do
+      it 'does not allow the move' do
         expect { subject }.to raise_error
       end
     end

ee/spec/models/boards/epic_board_position_spec.rb

@@ -46,18 +46,11 @@ RSpec.describe Boards::EpicBoardPosition do
     let_it_be(:position1) { create(:epic_board_position, relative_position: 1, epic_board: epic_board) }
     let_it_be(:position2) { create(:epic_board_position, relative_position: 1900, epic_board: epic_board) }
     let_it_be(:position3) { create(:epic_board_position, relative_position: 4000) }
+    let_it_be(:position4) { create(:epic_board_position, epic_board: epic_board, relative_position: nil) }
 
-    it 'returns the correct record' do
+    it 'returns highest not null position' do
       expect(described_class.last_for_board_id(epic_board.id)).to eq(position2)
     end
-
-    context 'with null relative_position record' do
-      let_it_be(:position4) { create(:epic_board_position, epic_board: epic_board, relative_position: nil) }
-
-      it 'returns the correct record' do
-        expect(described_class.last_for_board_id(epic_board.id)).to eq(position4)
-      end
-    end
   end
 
   context 'relative positioning' do

ee/spec/models/epic_spec.rb

@@ -47,18 +47,43 @@ RSpec.describe Epic do
       end
     end
 
-    describe '.order_relative_position_on_board' do
+    describe 'relative position scopes' do
       let_it_be(:board) { create(:epic_board) }
+      let_it_be(:other_board) { create(:epic_board) }
       let_it_be(:epic1) { create(:epic) }
       let_it_be(:epic2) { create(:epic) }
       let_it_be(:epic3) { create(:epic) }
 
-      it 'returns epics ordered by position on the board, null last' do
-        create(:epic_board_position, epic: epic2, epic_board: board, relative_position: 10)
-        create(:epic_board_position, epic: epic1, epic_board: board, relative_position: 20)
-        create(:epic_board_position, epic: epic3, epic_board: board, relative_position: 20)
+      let_it_be(:position1) { create(:epic_board_position, epic: epic1, epic_board: board, relative_position: 20) }
+      let_it_be(:position2) { create(:epic_board_position, epic: epic2, epic_board: board, relative_position: 10) }
+      let_it_be(:position3) { create(:epic_board_position, epic: epic3, epic_board: board, relative_position: 20) }
+      # this position should be ignored because it's for other board:
+      let_it_be(:position5) { create(:epic_board_position, epic: confidential_epic, epic_board: other_board, relative_position: 5) }
 
-        expect(described_class.order_relative_position_on_board(board.id)).to eq([epic2, epic3, epic1, public_epic, confidential_epic])
+      describe '.order_relative_position_on_board' do
+        it 'returns epics ordered by position on the board, null last' do
+          epics = described_class.order_relative_position_on_board(board.id)
+
+          expect(epics).to eq([epic2, epic3, epic1, public_epic, confidential_epic])
+        end
+      end
+
+      describe 'without_board_position' do
+        it 'returns only epics which do not have position set for the board' do
+          epics = described_class.join_board_position(board.id).without_board_position(board.id)
+
+          expect(epics).to match_array([confidential_epic, public_epic])
+        end
+      end
+
+      describe '.join_board_position' do
+        it 'returns epics with joined position for the board' do
+          positions = described_class.join_board_position(board.id)
+            .select('boards_epic_board_positions.relative_position as pos').map(&:pos)
+
+          # confidential_epic and public_epic should have both nil position for the board
+          expect(positions).to match_array([20, 10, 20, nil, nil])
+        end
       end
     end
 
@@ -87,15 +112,6 @@ RSpec.describe Epic do
         expect(described_class.from_id(epic2.id)).to match_array([epic2, epic3])
       end
     end
-
-    describe 'without_board_position' do
-      let_it_be(:epic1) { create(:epic, group: group) }
-      let_it_be(:position) { create(:epic_board_position, epic: epic1) }
-
-      it 'returns only epics without a board position record' do
-        expect(described_class.without_board_position).to match_array([confidential_epic, public_epic])
-      end
-    end
   end
 
   describe 'validations' do

ee/spec/requests/api/graphql/mutations/boards/epic_boards/epic_move_list_spec.rb

@@ -41,7 +41,7 @@ RSpec.describe 'Reposition and move epic between board lists' do
       group.add_developer(current_user)
     end
 
-    it 'raises resource not available error' do
+    it 'raises feature not available error' do
       subject
 
       expect(graphql_errors).to include(a_hash_including('message' => 'epic_boards feature is disabled'))
@@ -154,10 +154,8 @@ RSpec.describe 'Reposition and move epic between board lists' do
           iid,
           relativePosition
           labels {
-            edges {
-              node{
-                title
-              }
+            nodes {
+              title
             }
           }
         }

ee/spec/services/epics/update_service_spec.rb

@@ -115,8 +115,8 @@ RSpec.describe Epics::UpdateService do
       end
 
       shared_examples 'board repositioning' do
-        context 'when moving beetween 2 epics on the board' do
-          subject { update_epic(move_between_ids: [epic1.id, epic2.id], board_id: board.id, list_id: list.id) }
+        context 'when moving between 2 epics on the board' do
+          subject { update_epic(move_between_ids: [epic1.id, epic2.id], board_id: board.id, list_id: list.id, board_group: group) }
 
           it 'moves the epic correctly' do
             subject
@@ -130,7 +130,7 @@ RSpec.describe Epics::UpdateService do
 
         context 'when moving the epic to the end' do
           it 'moves the epic correctly' do
-            update_epic(move_between_ids: [nil, epic2.id], board_id: board.id, list_id: list.id)
+            update_epic(move_between_ids: [nil, epic2.id], board_id: board.id, list_id: list.id, board_group: group)
 
             expect(position(epic)).to be > position(epic2)
           end
@@ -147,7 +147,7 @@ RSpec.describe Epics::UpdateService do
 
         context 'when moving beetween 2 epics on the board' do
           it 'keeps epic3 on top of the board' do
-            update_epic(move_between_ids: [epic1.id, epic2.id], board_id: board.id, list_id: list.id)
+            update_epic(move_between_ids: [epic1.id, epic2.id], board_id: board.id, list_id: list.id, board_group: group)
 
             expect(position(epic3)).to be < position(epic2)
             expect(position(epic3)).to be < position(epic1)
@@ -160,7 +160,7 @@ RSpec.describe Epics::UpdateService do
           end
 
           it 'moves the epic correctly' do
-            update_epic(move_between_ids: [epic3.id, nil], board_id: board.id, list_id: list.id)
+            update_epic(move_between_ids: [epic3.id, nil], board_id: board.id, list_id: list.id, board_group: group)
 
             expect(epic_position.reload.relative_position).to be < epic3_position.relative_position
           end
@@ -168,7 +168,7 @@ RSpec.describe Epics::UpdateService do
 
         context 'when moving the epic to the end' do
           it 'keeps epic3 on top of the board' do
-            update_epic(move_between_ids: [epic1.id, epic2.id], board_id: board.id, list_id: list.id)
+            update_epic(move_between_ids: [epic1.id, epic2.id], board_id: board.id, list_id: list.id, board_group: group)
 
             expect(position(epic3)).to be < position(epic2)
             expect(position(epic3)).to be < position(epic1)
@@ -181,6 +181,13 @@ RSpec.describe Epics::UpdateService do
           it_behaves_like 'board repositioning'
         end
 
+        context 'when the epic is in a subgroup' do
+          let(:subgroup) { create(:group, parent: group) }
+          let(:epic) { create(:epic, group: subgroup) }
+
+          it_behaves_like 'board repositioning'
+        end
+
         context 'when the position does not exist for the record being moved' do
           let_it_be_with_reload(:epic1_position) { create(:epic_board_position, epic: epic1, epic_board: board, relative_position: 30) }
           let_it_be_with_reload(:epic2_position) { create(:epic_board_position, epic: epic2, epic_board: board, relative_position: 20) }
@@ -192,7 +199,7 @@ RSpec.describe Epics::UpdateService do
           let_it_be_with_reload(:epic2_position) { create(:epic_board_position, epic: epic2, epic_board: board, relative_position: 30) }
           let_it_be_with_reload(:epic_position) { create(:epic_board_position, epic: epic, epic_board: board, relative_position: 10) }
 
-          subject { update_epic(move_between_ids: [epic1.id, epic2.id], board_id: board.id, list_id: list.id) }
+          subject { update_epic(move_between_ids: [epic1.id, epic2.id], board_id: board.id, list_id: list.id, board_group: group) }
 
           it 'moves the epic correctly' do
             subject