Fix filling of missing position for closed lists

Assures that board relative position table is joined also when
filling missing positions in closed lists. Join with this table was
hooked to order_relative_position_on_board, but for closed lists
this scope is not used.

app/services/boards/base_items_list_service.rb

@@ -6,9 +6,9 @@ module Boards
     include ActiveRecord::ConnectionAdapters::Quoting
 
     def execute
-      return items.order_closed_date_desc if list&.closed?
+      items = init_collection
 
-      ordered_items
+      order(items)
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
@@ -17,7 +17,7 @@ module Boards
       keys = metadata_fields.keys
       # TODO: eliminate need for SQL literal fragment
       columns = Arel.sql(metadata_fields.values_at(*keys).join(', '))
-      results = item_model.where(id: items.select(issuables[:id])).pluck(columns)
+      results = item_model.where(id: init_collection.select(issuables[:id])).pluck(columns)
 
       Hash[keys.zip(results.flatten)]
     end
@@ -29,7 +29,7 @@ module Boards
       { size: 'COUNT(*)' }
     end
 
-    def ordered_items
+    def order(items)
       raise NotImplementedError
     end
 
@@ -47,8 +47,8 @@ module Boards
 
     # We memoize the query here since the finder methods we use are quite complex. This does not memoize the result of the query.
     # rubocop: disable CodeReuse/ActiveRecord
-    def items
-      strong_memoize(:items) do
+    def init_collection
+      strong_memoize(:init_collection) do
         filter(finder.execute).reorder(nil)
       end
     end

app/services/boards/issues/list_service.rb

@@ -11,7 +11,9 @@ module Boards
 
       private
 
-      def ordered_items
+      def order(items)
+        return items.order_closed_date_desc if list&.closed?
+
         items.order_by_position_and_priority(with_cte: params[:search].present?)
       end
 

ee/app/models/ee/epic.rb

@@ -129,8 +129,7 @@ module EE
       end
 
       scope :order_relative_position_on_board, ->(board_id) do
-        join_board_position(board_id)
-          .reorder(::Gitlab::Database.nulls_last_order('boards_epic_board_positions.relative_position', 'ASC'), 'epics.id DESC')
+        reorder(::Gitlab::Database.nulls_last_order('boards_epic_board_positions.relative_position', 'ASC'), 'epics.id DESC')
       end
 
       scope :without_board_position, ->(board_id) do

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

@@ -9,20 +9,47 @@ module Boards
         EpicsFinder.new(current_user, filter_params.merge(group_id: parent.id))
       end
 
-      def filter(items)
-        return super unless params[:from_id].present?
+      def filter(collection)
+        items = filter_by_from_id(collection)
+        items = filter_by_position_presence(items)
 
-        super.from_id(params[:from_id])
+        super(items)
+      end
+
+      def filter_by_from_id(items)
+        return items unless params[:from_id].present?
+
+        items.from_id(params[:from_id])
+      end
+
+      def filter_by_position_presence(items)
+        return items unless exclude_positioned?
+
+        items.join_board_position(board.id).without_board_position(board.id)
+      end
+
+      def exclude_positioned?
+        params[:exclude_positioned].present?
       end
 
       def board
         @board ||= parent.epic_boards.find(params[:board_id])
       end
 
-      def ordered_items
+      def order(items)
+        items = items.join_board_position(board.id) if needs_board_position?
+
+        return items.order_closed_date_desc if list&.closed?
+
         items.order_relative_position_on_board(board.id)
       end
 
+      def needs_board_position?
+        # we need to join board's relative position only for unclosed lists
+        # or if we are filling missing positions in the list
+        exclude_positioned? || !list&.closed?
+      end
+
       def item_model
         ::Epic
       end

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

@@ -47,10 +47,9 @@ module Boards
         # 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] }
+        list_params = { board_id: board_id, id: list_id, from_id: params[:from_id], exclude_positioned: true }
 
         Boards::Epics::ListService.new(parent, current_user, list_params).execute
-          .without_board_position(board_id)
           .select(:id)
           .limit(LIMIT)
       end

ee/spec/models/epic_spec.rb

@@ -62,7 +62,7 @@ RSpec.describe Epic do
 
       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)
+          epics = described_class.join_board_position(board.id).order_relative_position_on_board(board.id)
 
           expect(epics).to eq([epic2, epic3, epic1, public_epic, confidential_epic])
         end

ee/spec/services/epics/update_service_spec.rb

@@ -99,9 +99,9 @@ RSpec.describe Epics::UpdateService do
     context 'when repositioning an epic on a board' do
       let_it_be(:group) { create(:group) }
       let_it_be(:epic) { create(:epic, group: group) }
-      let_it_be(:epic1) { create(:epic, group: group) }
-      let_it_be(:epic2) { create(:epic, group: group) }
-      let_it_be(:epic3) { create(:epic, group: group) }
+      let_it_be_with_reload(:epic1) { create(:epic, group: group) }
+      let_it_be_with_reload(:epic2) { create(:epic, group: group) }
+      let_it_be_with_reload(:epic3) { create(:epic, group: group) }
 
       let_it_be(:board) { create(:epic_board, group: group) }
       let_it_be(:list) { create(:epic_list, epic_board: board, list_type: :backlog) }
@@ -179,6 +179,18 @@ RSpec.describe Epics::UpdateService do
       context 'when board position records are missing' do
         context 'when the position does not exist for any record' do
           it_behaves_like 'board repositioning'
+
+          context 'when the list is closed' do
+            let_it_be(:list) { create(:epic_list, epic_board: board, list_type: :closed) }
+
+            before do
+              epic1.update!(state: :closed)
+              epic2.update!(state: :closed)
+              epic3.update!(state: :closed)
+            end
+
+            it_behaves_like 'board repositioning'
+          end
         end
 
         context 'when the epic is in a subgroup' do