Merge branch '338667-initialize-relative-positions-of-issues-appearing-in-board-list' into 'master'

Update relative positions on querying board issues

See merge request gitlab-org/gitlab!68715

app/controllers/boards/issues_controller.rb

@@ -27,9 +27,7 @@ module Boards
       list_service = Boards::Issues::ListService.new(board_parent, current_user, filter_params)
       issues = issues_from(list_service)
 
-      if Gitlab::Database.read_write? && !board.disabled_for?(current_user)
-        Issue.move_nulls_to_end(issues)
-      end
+      ::Boards::Issues::ListService.initialize_relative_positions(board, current_user, issues)
 
       render_issues(issues, list_service.metadata)
     end

app/graphql/resolvers/board_list_issues_resolver.rb

@@ -15,8 +15,11 @@ module Resolvers
     def resolve(**args)
       filter_params = item_filters(args[:filters]).merge(board_id: list.board.id, id: list.id)
       service = ::Boards::Issues::ListService.new(list.board.resource_parent, context[:current_user], filter_params)
+      pagination_connections = Gitlab::Graphql::Pagination::Keyset::Connection.new(service.execute)
 
-      service.execute
+      ::Boards::Issues::ListService.initialize_relative_positions(list.board, current_user, pagination_connections.items)
+
+      pagination_connections
     end
 
     # https://gitlab.com/gitlab-org/gitlab/-/issues/235681

app/services/boards/issues/list_service.rb

@@ -9,6 +9,14 @@ module Boards
         IssuesFinder.valid_params
       end
 
+      # It is a class method because we cannot apply it
+      # prior to knowing how many items should be fetched for a list.
+      def self.initialize_relative_positions(board, current_user, issues)
+        if Gitlab::Database.read_write? && !board.disabled_for?(current_user)
+          Issue.move_nulls_to_end(issues)
+        end
+      end
+
       private
 
       def order(items)

ee/spec/graphql/ee/resolvers/board_list_issues_resolver_spec.rb

@@ -134,6 +134,6 @@ RSpec.describe Resolvers::BoardListIssuesResolver do
   end
 
   def resolve_board_list_issues(args)
-    resolve(described_class, obj: list, args: args, ctx: { current_user: user })
+    resolve(described_class, obj: list, args: args, ctx: { current_user: user }).items
   end
 end

spec/graphql/resolvers/board_list_issues_resolver_spec.rb

@@ -20,18 +20,20 @@ RSpec.describe Resolvers::BoardListIssuesResolver do
       let!(:issue1) { create(:issue, project: project, labels: [label], relative_position: 10) }
       let!(:issue2) { create(:issue, project: project, labels: [label, label2], relative_position: 12) }
       let!(:issue3) { create(:issue, project: project, labels: [label, label3], relative_position: 10) }
+      let!(:issue4) { create(:issue, project: project, labels: [label], relative_position: nil) }
 
-      it 'returns the issues in the correct order' do
+      it 'returns issues in the correct order with non-nil relative positions', :aggregate_failures do
         # by relative_position and then ID
-        issues = resolve_board_list_issues
+        result = resolve_board_list_issues
 
-        expect(issues.map(&:id)).to eq [issue3.id, issue1.id, issue2.id]
+        expect(result.map(&:id)).to eq [issue3.id, issue1.id, issue2.id, issue4.id]
+        expect(result.map(&:relative_position)).not_to include(nil)
       end
 
       it 'finds only issues matching filters' do
         result = resolve_board_list_issues(args: { filters: { label_name: [label.title], not: { label_name: [label2.title] } } })
 
-        expect(result).to match_array([issue1, issue3])
+        expect(result).to match_array([issue1, issue3, issue4])
       end
 
       it 'finds only issues matching search param' do
@@ -49,7 +51,7 @@ RSpec.describe Resolvers::BoardListIssuesResolver do
       it 'accepts assignee wildcard id NONE' do
         result = resolve_board_list_issues(args: { filters: { assignee_wildcard_id: 'NONE' } })
 
-        expect(result).to match_array([issue1, issue2, issue3])
+        expect(result).to match_array([issue1, issue2, issue3, issue4])
       end
 
       it 'accepts assignee wildcard id ANY' do
@@ -89,6 +91,6 @@ RSpec.describe Resolvers::BoardListIssuesResolver do
   end
 
   def resolve_board_list_issues(args: {}, current_user: user)
-    resolve(described_class, obj: list, args: args, ctx: { current_user: current_user })
+    resolve(described_class, obj: list, args: args, ctx: { current_user: current_user }).items
   end
 end

spec/requests/api/graphql/boards/board_list_issues_query_spec.rb

@@ -48,13 +48,18 @@ RSpec.describe 'get board lists' do
     issues_data.map { |i| i['title'] }
   end
 
+  def issue_relative_positions
+    issues_data.map { |i| i['relativePosition'] }
+  end
+
   shared_examples 'group and project board list issues query' do
     let!(:board) { create(:board, resource_parent: board_parent) }
     let!(:label_list) { create(:list, board: board, label: label, position: 10) }
     let!(:issue1) { create(:issue, project: issue_project, labels: [label, label2], relative_position: 9) }
     let!(:issue2) { create(:issue, project: issue_project, labels: [label, label2], relative_position: 2) }
-    let!(:issue3) { create(:issue, project: issue_project, labels: [label], relative_position: 9) }
-    let!(:issue4) { create(:issue, project: issue_project, labels: [label2], relative_position: 432) }
+    let!(:issue3) { create(:issue, project: issue_project, labels: [label, label2], relative_position: nil) }
+    let!(:issue4) { create(:issue, project: issue_project, labels: [label], relative_position: 9) }
+    let!(:issue5) { create(:issue, project: issue_project, labels: [label2], relative_position: 432) }
 
     context 'when the user does not have access to the board' do
       it 'returns nil' do
@@ -69,10 +74,11 @@ RSpec.describe 'get board lists' do
         board_parent.add_reporter(user)
       end
 
-      it 'can access the issues' do
+      it 'can access the issues', :aggregate_failures do
         post_graphql(query("id: \"#{global_id_of(label_list)}\""), current_user: user)
 
-        expect(issue_titles).to eq([issue2.title, issue1.title])
+        expect(issue_titles).to eq([issue2.title, issue1.title, issue3.title])
+        expect(issue_relative_positions).not_to include(nil)
       end
     end
   end

spec/services/boards/issues/list_service_spec.rb

@@ -139,4 +139,51 @@ RSpec.describe Boards::Issues::ListService do
     end
     # rubocop: enable RSpec/MultipleMemoizedHelpers
   end
+
+  describe '.initialize_relative_positions' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project) { create(:project, :empty_repo) }
+    let_it_be(:board) { create(:board, project: project) }
+    let_it_be(:backlog) { create(:backlog_list, board: board) }
+
+    let(:issue) { create(:issue, project: project, relative_position: nil) }
+
+    context "when 'Gitlab::Database::read_write?' is true" do
+      before do
+        allow(Gitlab::Database).to receive(:read_write?).and_return(true)
+      end
+
+      context 'user cannot move issues' do
+        it 'does not initialize the relative positions of issues' do
+          described_class.initialize_relative_positions(board, user, [issue])
+
+          expect(issue.relative_position).to eq nil
+        end
+      end
+
+      context 'user can move issues' do
+        before do
+          project.add_developer(user)
+        end
+
+        it 'initializes the relative positions of issues' do
+          described_class.initialize_relative_positions(board, user, [issue])
+
+          expect(issue.relative_position).not_to eq nil
+        end
+      end
+    end
+
+    context "when 'Gitlab::Database::read_write?' is false" do
+      before do
+        allow(Gitlab::Database).to receive(:read_write?).and_return(false)
+      end
+
+      it 'does not initialize the relative positions of issues' do
+        described_class.initialize_relative_positions(board, user, [issue])
+
+        expect(issue.relative_position).to eq nil
+      end
+    end
+  end
 end