Merge branch 'block-issue-positioning' into 'master'

Allow to block issues mass positioning [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!60141

app/controllers/boards/issues_controller.rb

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

app/helpers/boards_helper.rb

@@ -10,7 +10,7 @@ module BoardsHelper
       boards_endpoint: @boards_endpoint,
       lists_endpoint: board_lists_path(board),
       board_id: board.id,
-      disabled: disabled?.to_s,
+      disabled: board.disabled_for?(current_user).to_s,
       root_path: root_path,
       full_path: full_path,
       bulk_update_path: @bulk_issues_path,
@@ -105,10 +105,6 @@ module BoardsHelper
     can?(current_user, :admin_issue, current_board_parent)
   end
 
-  def disabled?
-    !can?(current_user, :create_non_backlog_issues, board)
-  end
-
   def board_list_data
     include_descendant_groups = @group&.present?
 

app/helpers/issues_helper.rb

@@ -9,6 +9,22 @@ module IssuesHelper
     classes.join(' ')
   end
 
+  def issue_manual_ordering_class
+    is_sorting_by_relative_position = @sort == 'relative_position'
+
+    if is_sorting_by_relative_position && !issue_repositioning_disabled?
+      "manual-ordering"
+    end
+  end
+
+  def issue_repositioning_disabled?
+    if @group
+      @group.root_ancestor.issue_repositioning_disabled?
+    elsif @project
+      @project.root_namespace.issue_repositioning_disabled?
+    end
+  end
+
   def status_box_class(item)
     if item.try(:expired?)
       'status-box-expired'

app/models/board.rb

@@ -45,6 +45,12 @@ class Board < ApplicationRecord
   def to_type
     self.class.to_type
   end
+
+  def disabled_for?(current_user)
+    namespace = group_board? ? resource_parent.root_ancestor : resource_parent.root_namespace
+
+    namespace.issue_repositioning_disabled? || !Ability.allowed?(current_user, :create_non_backlog_issues, self)
+  end
 end
 
 Board.prepend_mod_with('Board')

app/models/concerns/relative_positioning.rb

@@ -79,6 +79,8 @@ module RelativePositioning
       objects = objects.reject(&:relative_position)
       return 0 if objects.empty?
 
+      objects.first.check_repositioning_allowed!
+
       number_of_gaps = objects.size # 1 to the nearest neighbour, and one between each
       representative = RelativePositioning.mover.context(objects.first)
 
@@ -123,6 +125,12 @@ module RelativePositioning
     ::Gitlab::RelativePositioning::Mover.new(START_POSITION, (MIN_POSITION..MAX_POSITION))
   end
 
+  # To be overriden on child classes whenever
+  # blocking position updates is necessary.
+  def check_repositioning_allowed!
+    nil
+  end
+
   def move_between(before, after)
     before, after = [before, after].sort_by(&:relative_position) if before && after
 

app/models/issue.rb

@@ -272,6 +272,18 @@ class Issue < ApplicationRecord
               "id DESC")
   end
 
+  # Temporary disable moving null elements because of performance problems
+  # For more information check https://gitlab.com/gitlab-com/gl-infra/production/-/issues/4321
+  def check_repositioning_allowed!
+    if blocked_for_repositioning?
+      raise ::Gitlab::RelativePositioning::IssuePositioningDisabled, "Issue relative position changes temporarily disabled."
+    end
+  end
+
+  def blocked_for_repositioning?
+    resource_parent.root_namespace&.issue_repositioning_disabled?
+  end
+
   def hook_attrs
     Gitlab::HookData::IssueBuilder.new(self).build
   end

app/models/namespace.rb

@@ -425,6 +425,10 @@ class Namespace < ApplicationRecord
     created_at >= 90.days.ago
   end
 
+  def issue_repositioning_disabled?
+    Feature.enabled?(:block_issue_repositioning, self, type: :ops, default_enabled: :yaml)
+  end
+
   private
 
   def expire_child_caches

app/services/issues/update_service.rb

@@ -100,6 +100,8 @@ module Issues
     end
 
     def handle_move_between_ids(issue)
+      issue.check_repositioning_allowed! if params[:move_between_ids]
+
       super
 
       rebalance_if_needed(issue)

app/views/groups/boards/show.html.haml

+= render 'shared/alerts/positioning_disabled'
+
 = render "shared/boards/show", board: @board, group: true

app/views/projects/issues/_issues.html.haml

 - is_project_overview = local_assigns.fetch(:is_project_overview, false)
+= render 'shared/alerts/positioning_disabled'
 
 - if Feature.enabled?(:vue_issuables_list, @project) && !is_project_overview
   - data_endpoint = local_assigns.fetch(:data_endpoint, expose_path(api_v4_projects_issues_path(id: @project.id)))
@@ -15,7 +16,7 @@
     'scoped-labels-available': scoped_labels_available?(@project).to_json } }
 - else
   - empty_state_path = local_assigns.fetch(:empty_state_path, 'shared/empty_states/issues')
-  %ul.content-list.issues-list.issuable-list{ class: ("manual-ordering" if @sort == 'relative_position') }
+  %ul.content-list.issues-list.issuable-list{ class: issue_manual_ordering_class }
     = render partial: "projects/issues/issue", collection: @issues
     - if @issues.blank?
       = render empty_state_path

app/views/shared/_issues.html.haml

+= render 'shared/alerts/positioning_disabled'
+
 - if @issues.to_a.any?
-  %ul.content-list.issues-list.issuable-list{ class: ("manual-ordering" if @sort == 'relative_position'), data: { group_full_path: @group&.full_path } }
+  %ul.content-list.issues-list.issuable-list{ class: issue_manual_ordering_class, data: { group_full_path: @group&.full_path } }
     = render partial: 'projects/issues/issue', collection: @issues
   = paginate @issues, theme: "gitlab"
 - else

app/views/shared/alerts/_positioning_disabled.html.haml

+- if issue_repositioning_disabled?
+  = render 'shared/alert_info', body: _('Issues manual ordering is temporarily disabled for technical reasons.')

app/views/shared/boards/_show.html.haml

@@ -7,6 +7,8 @@
   - breadcrumb_title _("Epic Boards")
 - else
   - breadcrumb_title _("Issue Boards")
+  = render 'shared/alerts/positioning_disabled'
+
 - page_title("#{board.name}", _("Boards"))
 - add_page_specific_style 'page_bundles/boards'
 

app/workers/issue_placement_worker.rb

@@ -20,6 +20,10 @@ class IssuePlacementWorker
     issue = find_issue(issue_id, project_id)
     return unless issue
 
+    # Temporary disable moving null elements because of performance problems
+    # For more information check https://gitlab.com/gitlab-com/gl-infra/production/-/issues/4321
+    return if issue.blocked_for_repositioning?
+
     # Move the oldest 100 unpositioned items to the end.
     # This is to deal with out-of-order execution of the worker,
     # while preserving creation order.

app/workers/issue_rebalancing_worker.rb

@@ -14,6 +14,11 @@ class IssueRebalancingWorker
     return if project_id.nil?
 
     project = Project.find(project_id)
+
+    # Temporary disable reabalancing for performance reasons
+    # For more information check https://gitlab.com/gitlab-com/gl-infra/production/-/issues/4321
+    return if project.root_namespace&.issue_repositioning_disabled?
+
     # All issues are equivalent as far as we are concerned
     issue = project.issues.take # rubocop: disable CodeReuse/ActiveRecord
 

config/feature_flags/ops/block_issue_repositioning.yml

+---
+name: block_issue_repositioning
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60141
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/329663
+milestone: '13.12'
+type: ops
+group: group::project management
+default_enabled: false

ee/app/helpers/ee/boards_helper.rb

@@ -10,6 +10,7 @@ module EE
                   list_assignees_path: board_users_path(board, :json))
     end
 
+    # rubocop:disable Metrics/AbcSize
     override :board_data
     def board_data
       show_feature_promotion = @project && show_promotions? &&
@@ -28,7 +29,7 @@ module EE
         show_promotion: show_feature_promotion,
         can_update: can_update?.to_s,
         can_admin_list: can_admin_list?.to_s,
-        disabled: disabled?.to_s,
+        disabled: board.disabled_for?(current_user).to_s,
         emails_disabled: current_board_parent.emails_disabled?.to_s
       }
 
@@ -53,6 +54,7 @@ module EE
         iteration_feature_available: current_board_namespace.feature_available?(:iterations).to_s
       }
     end
+    # rubocop:enable Metrics/AbcSize
 
     override :can_update?
     def can_update?
@@ -75,13 +77,6 @@ module EE
       super
     end
 
-    override :disabled?
-    def disabled?
-      return false if board.is_a?(::Boards::EpicBoard)
-
-      super
-    end
-
     override :recent_boards_path
     def recent_boards_path
       return recent_group_boards_path(@group) if current_board_parent.is_a?(Group)

ee/app/models/boards/epic_board.rb

@@ -64,5 +64,9 @@ module Boards
     def weight
       nil
     end
+
+    def disabled_for?(current_user)
+      false
+    end
   end
 end

lib/gitlab/relative_positioning.rb

@@ -15,6 +15,7 @@ module Gitlab
     NoSpaceLeft = Class.new(StandardError)
     InvalidPosition = Class.new(StandardError)
     IllegalRange = Class.new(ArgumentError)
+    IssuePositioningDisabled = Class.new(StandardError)
 
     def self.range(lhs, rhs)
       if lhs && rhs

locale/gitlab.pot

@@ -18359,6 +18359,9 @@ msgstr ""
 msgid "Issues closed"
 msgstr ""
 
+msgid "Issues manual ordering is temporarily disabled for technical reasons."
+msgstr ""
+
 msgid "Issues must match this scope to appear in this list."
 msgstr ""
 

spec/controllers/boards/issues_controller_spec.rb

@@ -49,6 +49,7 @@ RSpec.describe Boards::IssuesController do
           create(:labeled_issue, project: project, labels: [development], due_date: Date.tomorrow)
           create(:labeled_issue, project: project, labels: [development], assignees: [johndoe])
           issue.subscribe(johndoe, project)
+          expect(Issue).to receive(:move_nulls_to_end)
 
           list_issues user: user, board: board, list: list2
 
@@ -119,6 +120,18 @@ RSpec.describe Boards::IssuesController do
 
           expect(query_count).to eq(1)
         end
+
+        context 'when block_issue_repositioning feature flag is enabled' do
+          before do
+            stub_feature_flags(block_issue_repositioning: true)
+          end
+
+          it 'does not reposition issues with null position' do
+            expect(Issue).not_to receive(:move_nulls_to_end)
+
+            list_issues(user: user, board: group_board, list: list3)
+          end
+        end
       end
 
       context 'with invalid list id' do

spec/helpers/issues_helper_spec.rb

@@ -341,4 +341,65 @@ RSpec.describe IssuesHelper do
       end
     end
   end
+
+  describe '#issue_manual_ordering_class' do
+    context 'when sorting by relative position' do
+      before do
+        assign(:sort, 'relative_position')
+      end
+
+      it 'returns manual ordering class' do
+        expect(helper.issue_manual_ordering_class).to eq("manual-ordering")
+      end
+
+      context 'when manual sorting disabled' do
+        before do
+          allow(helper).to receive(:issue_repositioning_disabled?).and_return(true)
+        end
+
+        it 'returns nil' do
+          expect(helper.issue_manual_ordering_class).to eq(nil)
+        end
+      end
+    end
+  end
+
+  describe '#issue_repositioning_disabled?' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:project) { create(:project, group: group) }
+
+    subject { helper.issue_repositioning_disabled? }
+
+    context 'for project' do
+      before do
+        assign(:project, project)
+      end
+
+      it { is_expected.to eq(false) }
+
+      context 'when block_issue_repositioning feature flag is enabled' do
+        before do
+          stub_feature_flags(block_issue_repositioning: group)
+        end
+
+        it { is_expected.to eq(true) }
+      end
+    end
+
+    context 'for group' do
+      before do
+        assign(:group, group)
+      end
+
+      it { is_expected.to eq(false) }
+
+      context 'when block_issue_repositioning feature flag is enabled' do
+        before do
+          stub_feature_flags(block_issue_repositioning: group)
+        end
+
+        it { is_expected.to eq(true) }
+      end
+    end
+  end
 end

spec/models/board_spec.rb

@@ -42,4 +42,46 @@ RSpec.describe Board do
       expect { project.boards.first_board.find(board_A.id) }.to raise_error(ActiveRecord::RecordNotFound)
     end
   end
+
+  describe '#disabled_for?' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:project) { create(:project, group: group) }
+    let_it_be(:user) { create(:user) }
+
+    subject { board.disabled_for?(user) }
+
+    shared_examples 'board disabled_for?' do
+      context 'when current user cannot create non backlog issues' do
+        it { is_expected.to eq(true) }
+      end
+
+      context 'when user can create backlog issues' do
+        before do
+          board.resource_parent.add_reporter(user)
+        end
+
+        it { is_expected.to eq(false) }
+
+        context 'when block_issue_repositioning is enabled' do
+          before do
+            stub_feature_flags(block_issue_repositioning: group)
+          end
+
+          it { is_expected.to eq(true) }
+        end
+      end
+    end
+
+    context 'for group board' do
+      let_it_be(:board) { create(:board, group: group) }
+
+      it_behaves_like 'board disabled_for?'
+    end
+
+    context 'for project board' do
+      let_it_be(:board) { create(:board, project: project) }
+
+      it_behaves_like 'board disabled_for?'
+    end
+  end
 end

spec/models/issue_spec.rb

@@ -1141,11 +1141,37 @@ RSpec.describe Issue do
   end
 
   context "relative positioning" do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:project) { create(:project, group: group) }
+    let_it_be(:issue1) { create(:issue, project: project, relative_position: nil) }
+    let_it_be(:issue2) { create(:issue, project: project, relative_position: nil) }
+
     it_behaves_like "a class that supports relative positioning" do
       let_it_be(:project) { reusable_project }
       let(:factory) { :issue }
       let(:default_params) { { project: project } }
     end
+
+    it 'is not blocked for repositioning by default' do
+      expect(issue1.blocked_for_repositioning?).to eq(false)
+    end
+
+    context 'when block_issue_repositioning flag is enabled for group' do
+      before do
+        stub_feature_flags(block_issue_repositioning: group)
+      end
+
+      it 'is blocked for repositioning' do
+        expect(issue1.blocked_for_repositioning?).to eq(true)
+      end
+
+      it 'does not move issues with null position' do
+        payload = [issue1, issue2]
+
+        expect { described_class.move_nulls_to_end(payload) }.to raise_error(Gitlab::RelativePositioning::IssuePositioningDisabled)
+        expect { described_class.move_nulls_to_start(payload) }.to raise_error(Gitlab::RelativePositioning::IssuePositioningDisabled)
+      end
+    end
   end
 
   it_behaves_like 'versioned description'

spec/services/issues/update_service_spec.rb

@@ -165,20 +165,38 @@ RSpec.describe Issues::UpdateService, :mailer do
         expect(user2.assigned_open_issues_count).to eq 1
       end
 
-      it 'sorts issues as specified by parameters' do
-        issue1 = create(:issue, project: project, assignees: [user3])
-        issue2 = create(:issue, project: project, assignees: [user3])
+      context 'when changing relative position' do
+        let(:issue1) { create(:issue, project: project, assignees: [user3]) }
+        let(:issue2) { create(:issue, project: project, assignees: [user3]) }
 
-        [issue, issue1, issue2].each do |issue|
-          issue.move_to_end
-          issue.save!
+        before do
+          [issue, issue1, issue2].each do |issue|
+            issue.move_to_end
+            issue.save!
+          end
         end
 
-        opts[:move_between_ids] = [issue1.id, issue2.id]
+        it 'sorts issues as specified by parameters' do
+          opts[:move_between_ids] = [issue1.id, issue2.id]
 
-        update_issue(opts)
+          update_issue(opts)
 
-        expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
+          expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
+        end
+
+        context 'when block_issue_positioning flag is enabled' do
+          before do
+            stub_feature_flags(block_issue_repositioning: true)
+          end
+
+          it 'raises error' do
+            old_position = issue.relative_position
+            opts[:move_between_ids] = [issue1.id, issue2.id]
+
+            expect { update_issue(opts) }.to raise_error(::Gitlab::RelativePositioning::IssuePositioningDisabled)
+            expect(issue.reload.relative_position).to eq(old_position)
+          end
+        end
       end
 
       it 'does not rebalance even if needed if the flag is disabled' do

spec/spec_helper.rb

@@ -288,6 +288,12 @@ RSpec.configure do |config|
       # Selectively disable by actor https://docs.gitlab.com/ee/development/feature_flags/#selectively-disable-by-actor
       stub_feature_flags(remove_description_html_in_release_api_override: false)
 
+      # Disable issue respositioning to avoid heavy load on database when importing big projects.
+      # This is only turned on when app is handling heavy project imports.
+      # Can be removed when we find a better way to deal with the problem.
+      # For more information check https://gitlab.com/gitlab-com/gl-infra/production/-/issues/4321
+      stub_feature_flags(block_issue_repositioning: false)
+
       allow(Gitlab::GitalyClient).to receive(:can_use_disk?).and_return(enable_rugged)
     else
       unstub_all_feature_flags

spec/workers/issue_placement_worker_spec.rb

@@ -5,7 +5,8 @@ require 'spec_helper'
 RSpec.describe IssuePlacementWorker do
   describe '#perform' do
     let_it_be(:time) { Time.now.utc }
-    let_it_be(:project) { create(:project) }
+    let_it_be(:group) { create(:group) }
+    let_it_be(:project) { create(:project, group: group) }
     let_it_be(:author) { create(:user) }
     let_it_be(:common_attrs) { { author: author, project: project } }
     let_it_be(:unplaced) { common_attrs.merge(relative_position: nil) }
@@ -117,6 +118,19 @@ RSpec.describe IssuePlacementWorker do
       let(:worker_arguments) { { issue_id: issue_id, project_id: nil } }
 
       it_behaves_like 'running the issue placement worker'
+
+      context 'when block_issue_repositioning is enabled' do
+        let(:issue_id) { issue.id }
+        let(:project_id) { project.id }
+
+        before do
+          stub_feature_flags(block_issue_repositioning: group)
+        end
+
+        it 'does not run repositioning tasks' do
+          expect { run_worker }.not_to change { issue.reset.relative_position }
+        end
+      end
     end
 
     context 'passing a project ID' do

spec/workers/issue_rebalancing_worker_spec.rb

@@ -4,7 +4,21 @@ require 'spec_helper'
 
 RSpec.describe IssueRebalancingWorker do
   describe '#perform' do
-    let_it_be(:issue) { create(:issue) }
+    let_it_be(:group) { create(:group) }
+    let_it_be(:project) { create(:project, group: group) }
+    let_it_be(:issue) { create(:issue, project: project) }
+
+    context 'when block_issue_repositioning is enabled' do
+      before do
+        stub_feature_flags(block_issue_repositioning: group)
+      end
+
+      it 'does not run an instance of IssueRebalancingService' do
+        expect(IssueRebalancingService).not_to receive(:new)
+
+        described_class.new.perform(nil, issue.project_id)
+      end
+    end
 
     it 'runs an instance of IssueRebalancingService' do
       service = double(execute: nil)