Merge branch 'optimize-issue-board-lists' into 'master'

Remove priority sort from board list issues

See merge request gitlab-org/gitlab!70674

app/models/issue.rb

@@ -107,8 +107,6 @@ class Issue < ApplicationRecord
   scope :order_due_date_asc, -> { reorder(::Gitlab::Database.nulls_last_order('due_date', 'ASC')) }
   scope :order_due_date_desc, -> { reorder(::Gitlab::Database.nulls_last_order('due_date', 'DESC')) }
   scope :order_closest_future_date, -> { reorder(Arel.sql('CASE WHEN issues.due_date >= CURRENT_DATE THEN 0 ELSE 1 END ASC, ABS(CURRENT_DATE - issues.due_date) ASC')) }
-  scope :order_relative_position_asc, -> { reorder(::Gitlab::Database.nulls_last_order('relative_position', 'ASC')) }
-  scope :order_relative_position_desc, -> { reorder(::Gitlab::Database.nulls_first_order('relative_position', 'DESC')) }
   scope :order_closed_date_desc, -> { reorder(closed_at: :desc) }
   scope :order_created_at_desc, -> { reorder(created_at: :desc) }
   scope :order_severity_asc, -> { includes(:issuable_severity).order('issuable_severities.severity ASC NULLS FIRST') }
@@ -267,8 +265,8 @@ class Issue < ApplicationRecord
         'due_date' => -> { order_due_date_asc.with_order_id_desc },
         'due_date_asc' => -> { order_due_date_asc.with_order_id_desc },
         'due_date_desc' => -> { order_due_date_desc.with_order_id_desc },
-        'relative_position' => -> { order_relative_position_asc.with_order_id_desc },
-        'relative_position_asc' => -> { order_relative_position_asc.with_order_id_desc }
+        'relative_position' => -> { order_by_relative_position },
+        'relative_position_asc' => -> { order_by_relative_position }
       }
     )
   end
@@ -278,7 +276,7 @@ class Issue < ApplicationRecord
     when 'closest_future_date', 'closest_future_date_asc' then order_closest_future_date
     when 'due_date', 'due_date_asc'                       then order_due_date_asc.with_order_id_desc
     when 'due_date_desc'                                  then order_due_date_desc.with_order_id_desc
-    when 'relative_position', 'relative_position_asc'     then order_relative_position_asc.with_order_id_desc
+    when 'relative_position', 'relative_position_asc'     then order_by_relative_position
     when 'severity_asc'                                   then order_severity_asc.with_order_id_desc
     when 'severity_desc'                                  then order_severity_desc.with_order_id_desc
     else
@@ -286,13 +284,8 @@ class Issue < ApplicationRecord
     end
   end
 
-  # `with_cte` argument allows sorting when using CTE queries and prevents
-  # errors in postgres when using CTE search optimisation
-  def self.order_by_position_and_priority(with_cte: false)
-    order = Gitlab::Pagination::Keyset::Order.build([column_order_relative_position, column_order_highest_priority, column_order_id_desc])
-
-    order_labels_priority(with_cte: with_cte)
-      .reorder(order)
+  def self.order_by_relative_position
+    reorder(Gitlab::Pagination::Keyset::Order.build([column_order_relative_position, column_order_id_asc]))
   end
 
   def self.column_order_relative_position
@@ -307,25 +300,6 @@ class Issue < ApplicationRecord
     )
   end
 
-  def self.column_order_highest_priority
-    Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
-      attribute_name: 'highest_priority',
-      column_expression: Arel.sql('highest_priorities.label_priority'),
-      order_expression: Gitlab::Database.nulls_last_order('highest_priorities.label_priority', 'ASC'),
-      reversed_order_expression: Gitlab::Database.nulls_last_order('highest_priorities.label_priority', 'DESC'),
-      order_direction: :asc,
-      nullable: :nulls_last,
-      distinct: false
-    )
-  end
-
-  def self.column_order_id_desc
-    Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
-      attribute_name: 'id',
-      order_expression: arel_table[:id].desc
-    )
-  end
-
   def self.column_order_id_asc
     Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
       attribute_name: 'id',

app/services/boards/issues/list_service.rb

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

app/services/issues/relative_position_rebalancing_service.rb

@@ -82,7 +82,7 @@ module Issues
       collection.each do |project|
         caching.cache_current_project_id(project.id)
         index += 1
-        scope = Issue.in_projects(project).reorder(custom_reorder).select(:id, :relative_position)
+        scope = Issue.in_projects(project).order_by_relative_position.select(:id, :relative_position)
 
         with_retry(PREFETCH_ISSUES_BATCH_SIZE, 100) do |batch_size|
           Gitlab::Pagination::Keyset::Iterator.new(scope: scope).each_batch(of: batch_size) do |batch|
@@ -166,10 +166,6 @@ module Issues
       @start_position ||= (RelativePositioning::START_POSITION - (gaps / 2) * gap_size).to_i
     end
 
-    def custom_reorder
-      ::Gitlab::Pagination::Keyset::Order.build([Issue.column_order_relative_position, Issue.column_order_id_asc])
-    end
-
     def with_retry(initial_batch_size, exit_batch_size)
       retries = 0
       batch_size = initial_batch_size

db/post_migrate/20210927153807_update_issues_relative_position_indexes.rb

+# frozen_string_literal: true
+
+class UpdateIssuesRelativePositionIndexes < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  RELATIVE_POSITION_INDEX_NAME = 'idx_issues_on_project_id_and_rel_asc_and_id'
+  RELATIVE_POSITION_STATE_INDEX_NAME = 'idx_issues_on_project_id_and_rel_position_and_state_id_and_id'
+
+  NEW_RELATIVE_POSITION_STATE_INDEX_NAME = 'idx_issues_on_project_id_and_rel_position_and_id_and_state_id'
+
+  def up
+    add_concurrent_index :issues, [:project_id, :relative_position, :id, :state_id], name: NEW_RELATIVE_POSITION_STATE_INDEX_NAME
+
+    remove_concurrent_index_by_name :issues, RELATIVE_POSITION_INDEX_NAME
+    remove_concurrent_index_by_name :issues, RELATIVE_POSITION_STATE_INDEX_NAME
+  end
+
+  def down
+    add_concurrent_index :issues, [:project_id, :relative_position, :state_id, :id], order: { id: :desc }, name: RELATIVE_POSITION_STATE_INDEX_NAME
+    add_concurrent_index :issues, [:project_id, :relative_position, :id], name: RELATIVE_POSITION_INDEX_NAME
+
+    remove_concurrent_index_by_name :issues, NEW_RELATIVE_POSITION_STATE_INDEX_NAME
+  end
+end

db/schema_migrations/20210927153807

+8e54f43a955023e422bf40476f468fbdf04f06e806b08fddf35208c65607fec3
\ No newline at end of file

db/structure.sql

@@ -24045,9 +24045,7 @@ CREATE INDEX idx_issues_on_project_id_and_created_at_and_id_and_state_id ON issu
 
 CREATE INDEX idx_issues_on_project_id_and_due_date_and_id_and_state_id ON issues USING btree (project_id, due_date, id, state_id) WHERE (due_date IS NOT NULL);
 
-CREATE INDEX idx_issues_on_project_id_and_rel_asc_and_id ON issues USING btree (project_id, relative_position, id);
-
-CREATE INDEX idx_issues_on_project_id_and_rel_position_and_state_id_and_id ON issues USING btree (project_id, relative_position, state_id, id DESC);
+CREATE INDEX idx_issues_on_project_id_and_rel_position_and_id_and_state_id ON issues USING btree (project_id, relative_position, id, state_id);
 
 CREATE INDEX idx_issues_on_project_id_and_updated_at_and_id_and_state_id ON issues USING btree (project_id, updated_at, id, state_id);
 

doc/user/project/issue_board.md

@@ -203,17 +203,13 @@ When visiting a board, issues appear ordered in any list. You're able to change
 that order by dragging the issues. The changed order is saved, so that anybody who visits the same
 board later sees the reordering, with some exceptions.
 
-The first time an issue appears in any board (that is, the first time a user
-loads a board containing that issue), it is ordered in relation to other issues in that list.
-The order is done according to [label priority](labels.md#label-priority).
+When an issue is created, the system assigns a relative order value that is greater than the maximum value
+of that issue's project or root group. This means the issue will be at the bottom of any issue list that
+it appears in.
 
-At this point, that issue is assigned a relative order value by the system,
-with respect to the other issues in the list. Any time
-you drag and reorder the issue, its relative order value changes accordingly.
-
-Also, any time that issue appears in any board, the ordering is done according to
-the updated relative order value. It's only the first
-time an issue appears that it takes from the priority order mentioned above. If a user in your GitLab instance
+Any time you drag and reorder the issue, its relative order value changes accordingly.
+Then, any time that issue appears in any board, the ordering is done according to
+the updated relative order value. If a user in your GitLab instance
 drags issue `A` above issue `B`, the ordering is maintained when these two issues are subsequently
 loaded in any board in the same instance. This could be a different project board or a different group
 board, for example.

ee/spec/features/boards/boards_spec.rb

@@ -81,8 +81,8 @@ RSpec.describe 'Project issue boards', :js do
   context 'total weight' do
     let!(:label) { create(:label, project: project, name: 'Label 1') }
     let!(:list) { create(:list, board: board, label: label, position: 0) }
-    let!(:issue) { create(:issue, project: project, weight: 3) }
-    let!(:issue_2) { create(:issue, project: project, weight: 2) }
+    let!(:issue) { create(:issue, project: project, weight: 3, relative_position: 2) }
+    let!(:issue_2) { create(:issue, project: project, weight: 2, relative_position: 1) }
 
     before do
       project.add_developer(user)

ee/spec/models/issuables_analytics_spec.rb

@@ -22,7 +22,7 @@ RSpec.describe IssuablesAnalytics do
 
     context 'when issuable relation is ordered by priority' do
       it 'generates chart data correctly' do
-        issues = project.issues.order_by_position_and_priority
+        issues = project.issues.order_labels_priority
         data = described_class.new(issuables: issues).data
 
         seed.each_pair do |months_back, issues_count|

spec/controllers/boards/issues_controller_spec.rb

@@ -116,7 +116,7 @@ RSpec.describe Boards::IssuesController do
 
         it 'does not query issues table more than once' do
           recorder = ActiveRecord::QueryRecorder.new { list_issues(user: user, board: board, list: list1) }
-          query_count = recorder.occurrences.select { |query,| query.start_with?('SELECT issues.*') }.each_value.first
+          query_count = recorder.occurrences.select { |query,| query.match?(/FROM "?issues"?/) }.each_value.first
 
           expect(query_count).to eq(1)
         end

spec/graphql/resolvers/board_list_issues_resolver_spec.rb

@@ -35,7 +35,7 @@ RSpec.describe Resolvers::BoardListIssuesResolver do
         # by relative_position and then ID
         result = resolve_board_list_issues
 
-        expect(result.map(&:id)).to eq [issue3.id, issue1.id, issue2.id, issue4.id]
+        expect(result.map(&:id)).to eq [issue1.id, issue3.id, issue2.id, issue4.id]
         expect(result.map(&:relative_position)).not_to include(nil)
       end
 

spec/graphql/resolvers/issues_resolver_spec.rb

@@ -338,7 +338,7 @@ RSpec.describe Resolvers::IssuesResolver do
           let_it_be(:relative_issue4) { create(:issue, project: project, relative_position: nil) }
 
           it 'sorts issues ascending' do
-            expect(resolve_issues(sort: :relative_position_asc).to_a).to eq [relative_issue3, relative_issue1, relative_issue4, relative_issue2]
+            expect(resolve_issues(sort: :relative_position_asc).to_a).to eq [relative_issue3, relative_issue1, relative_issue2, relative_issue4]
           end
         end
 

spec/lib/gitlab/import_export/json/streaming_serializer_spec.rb

@@ -115,7 +115,7 @@ RSpec.describe Gitlab::ImportExport::Json::StreamingSerializer do
         end
 
         it 'orders exported issues by custom column(relative_position)' do
-          expected_issues = exportable.issues.order_relative_position_desc.order(id: :desc).map(&:to_json)
+          expected_issues = exportable.issues.reorder(::Gitlab::Database.nulls_first_order('relative_position', 'DESC')).order(id: :desc).map(&:to_json)
 
           expect(json_writer).to receive(:write_relation_array).with(exportable_path, :issues, expected_issues)
 

spec/lib/gitlab/pagination/keyset/iterator_spec.rb

@@ -73,7 +73,7 @@ RSpec.describe Gitlab::Pagination::Keyset::Iterator do
 
             iterator.each_batch(of: 2) { |rel| positions.concat(rel.pluck(:relative_position, :id)) }
 
-            expect(positions).to eq(project.issues.order_relative_position_asc.order(id: :asc).pluck(:relative_position, :id))
+            expect(positions).to eq(project.issues.reorder(::Gitlab::Database.nulls_last_order('relative_position', 'ASC')).order(id: :asc).pluck(:relative_position, :id))
           end
         end
 
@@ -85,7 +85,7 @@ RSpec.describe Gitlab::Pagination::Keyset::Iterator do
 
             iterator.each_batch(of: 2) { |rel| positions.concat(rel.pluck(:relative_position, :id)) }
 
-            expect(positions).to eq(project.issues.order_relative_position_desc.order(id: :desc).pluck(:relative_position, :id))
+            expect(positions).to eq(project.issues.reorder(::Gitlab::Database.nulls_first_order('relative_position', 'DESC')).order(id: :desc).pluck(:relative_position, :id))
           end
         end
 

spec/models/issue_spec.rb

@@ -222,17 +222,15 @@ RSpec.describe Issue do
     end
   end
 
-  describe '#order_by_position_and_priority' do
+  describe '#order_by_relative_position' do
     let(:project) { reusable_project }
-    let(:p1) { create(:label, title: 'P1', project: project, priority: 1) }
-    let(:p2) { create(:label, title: 'P2', project: project, priority: 2) }
-    let!(:issue1) { create(:labeled_issue, project: project, labels: [p1]) }
-    let!(:issue2) { create(:labeled_issue, project: project, labels: [p2]) }
+    let!(:issue1) { create(:issue, project: project) }
+    let!(:issue2) { create(:issue, project: project) }
     let!(:issue3) { create(:issue, project: project, relative_position: -200) }
     let!(:issue4) { create(:issue, project: project, relative_position: -100) }
 
     it 'returns ordered list' do
-      expect(project.issues.order_by_position_and_priority)
+      expect(project.issues.order_by_relative_position)
         .to match [issue3, issue4, issue1, issue2]
     end
   end

spec/requests/api/graphql/project/issues_spec.rb

@@ -233,7 +233,7 @@ RSpec.describe 'getting an issue list for a project' do
           let(:all_records) do
             [
               relative_issue5.iid, relative_issue3.iid, relative_issue1.iid,
-              relative_issue4.iid, relative_issue2.iid
+              relative_issue2.iid, relative_issue4.iid
             ]
           end
         end

spec/workers/issue_placement_worker_spec.rb

@@ -27,7 +27,7 @@ RSpec.describe IssuePlacementWorker do
       it 'places all issues created at most 5 minutes before this one at the end, most recent last' do
         expect { run_worker }.not_to change { irrelevant.reset.relative_position }
 
-        expect(project.issues.order_relative_position_asc)
+        expect(project.issues.order_by_relative_position)
           .to eq([issue_e, issue_b, issue_a, issue, issue_c, issue_f, issue_d])
         expect(project.issues.where(relative_position: nil)).not_to exist
       end