Merge branch 'eboard-repos-fix' into 'master'

Fix filling of missing position for closed lists

See merge request gitlab-org/gitlab!63852

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