Refactor issue move to not require group_id param

This simplifies the code so that we allow passing in move_before_id /
move_after_id as long as they are part of the same project / namespace
hierarchy.

This also fixes the reorder API so that it now accepts IDs from other
projects within the same hierarchy.

Changelog: fixed

app/controllers/projects/issues_controller.rb

@@ -317,7 +317,7 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def reorder_params
-    params.permit(:move_before_id, :move_after_id, :group_full_path)
+    params.permit(:move_before_id, :move_after_id)
   end
 
   def store_uri

app/services/boards/base_item_move_service.rb

@@ -39,7 +39,7 @@ module Boards
     end
 
     def reposition_params(reposition_ids)
-      reposition_parent.merge(move_between_ids: reposition_ids)
+      { move_between_ids: reposition_ids }
     end
 
     def move_single_issuable(issuable, issuable_modification_params)

app/services/boards/issues/move_service.rb

@@ -54,10 +54,6 @@ module Boards
       def update(issue, issue_modification_params)
         ::Issues::UpdateService.new(project: issue.project, current_user: current_user, params: issue_modification_params).execute(issue)
       end
-
-      def reposition_parent
-        { board_group_id: board.group&.id }
-      end
     end
   end
 end

app/services/issuable_base_service.rb

@@ -495,10 +495,9 @@ class IssuableBaseService < ::BaseProjectService
     return unless params[:move_between_ids]
 
     after_id, before_id = params.delete(:move_between_ids)
-    positioning_scope_id = params.delete(positioning_scope_key)
 
-    issuable_before = issuable_for_positioning(before_id, positioning_scope_id)
-    issuable_after = issuable_for_positioning(after_id, positioning_scope_id)
+    issuable_before = issuable_for_positioning(before_id, positioning_scope)
+    issuable_after = issuable_for_positioning(after_id, positioning_scope)
 
     raise ActiveRecord::RecordNotFound unless issuable_before || issuable_after
 

app/services/issues/reorder_service.rb

@@ -4,9 +4,8 @@ module Issues
   class ReorderService < Issues::BaseService
     def execute(issue)
       return false unless can?(current_user, :update_issue, issue)
-      return false if group && !can?(current_user, :read_group, group)
 
-      attrs = issue_params(group)
+      attrs = issue_params
       return false if attrs.empty?
 
       update(issue, attrs)
@@ -14,24 +13,17 @@ module Issues
 
     private
 
-    def group
-      return unless params[:group_full_path]
-
-      @group ||= Group.find_by_full_path(params[:group_full_path])
-    end
-
     def update(issue, attrs)
       ::Issues::UpdateService.new(project: project, current_user: current_user, params: attrs).execute(issue)
     rescue ActiveRecord::RecordNotFound
       false
     end
 
-    def issue_params(group)
+    def issue_params
       attrs = {}
 
       if move_between_ids
         attrs[:move_between_ids] = move_between_ids
-        attrs[:board_group_id]   = group&.id
       end
 
       attrs

app/services/issues/update_service.rb

@@ -13,6 +13,8 @@ module Issues
     end
 
     def execute(issue)
+      @issue = issue
+
       handle_move_between_ids(issue)
 
       change_issue_duplicate(issue)
@@ -100,10 +102,6 @@ module Issues
       rebalance_if_needed(issue)
     end
 
-    def positioning_scope_key
-      :board_group_id
-    end
-
     # rubocop: disable CodeReuse/ActiveRecord
     def change_issue_duplicate(issue)
       canonical_issue_id = params.delete(:canonical_issue_id)
@@ -221,20 +219,17 @@ module Issues
       ).execute
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def issuable_for_positioning(id, board_group_id = nil)
+    def issuable_for_positioning(id, positioning_scope)
       return unless id
 
-      issue =
-        if board_group_id
-          IssuesFinder.new(current_user, group_id: board_group_id, include_subgroups: true).find_by(id: id)
-        else
-          project.issues.find(id)
-        end
+      issue = positioning_scope.find(id)
 
       issue if can?(current_user, :update_issue, issue)
     end
-    # rubocop: enable CodeReuse/ActiveRecord
+
+    def positioning_scope
+      Issue.relative_positioning_query_base(@issue)
+    end
 
     def create_confidentiality_note(issue)
       SystemNoteService.change_issue_confidentiality(issue, issue.project, current_user)

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

@@ -22,11 +22,7 @@ module Boards
 
       override :reposition_params
       def reposition_params(reposition_ids)
-        super.merge(list_id: params[:to_list_id], board_group: parent)
-      end
-
-      def reposition_parent
-        { board_id: board.id }
+        super.merge(list_id: params[:to_list_id], board_id: board.id, board_group: parent)
       end
     end
   end

ee/app/services/epics/update_service.rb

@@ -146,12 +146,10 @@ module Epics
     end
 
     def epic_board_id
-      params[positioning_scope_key]
+      params[:board_id]
     end
 
-    def positioning_scope_key
-      :board_id
-    end
+    alias_method :positioning_scope, :epic_board_id
 
     def saved_change_to_epic_dates?(epic)
       (epic.saved_changes.keys.map(&:to_sym) & EPIC_DATE_FIELDS).present?

spec/controllers/projects/issues_controller_spec.rb

@@ -502,10 +502,7 @@ RSpec.describe Projects::IssuesController do
 
       context 'with valid params' do
         it 'reorders issues and returns a successful 200 response' do
-          reorder_issue(issue1,
-            move_after_id: issue2.id,
-            move_before_id: issue3.id,
-            group_full_path: group.full_path)
+          reorder_issue(issue1, move_after_id: issue2.id, move_before_id: issue3.id)
 
           [issue1, issue2, issue3].map(&:reload)
 
@@ -531,12 +528,10 @@ RSpec.describe Projects::IssuesController do
         end
 
         it 'returns a unprocessable entity 422 response for issues not in group' do
-          another_group = create(:group)
+          other_group_project = create(:project, group: create(:group))
+          other_group_issue = create(:issue, project: other_group_project)
 
-          reorder_issue(issue1,
-            move_after_id: issue2.id,
-            move_before_id: issue3.id,
-            group_full_path: another_group.full_path)
+          reorder_issue(issue1, move_after_id: issue2.id, move_before_id: other_group_issue.id)
 
           expect(response).to have_gitlab_http_status(:unprocessable_entity)
         end
@@ -555,15 +550,14 @@ RSpec.describe Projects::IssuesController do
       end
     end
 
-    def reorder_issue(issue, move_after_id: nil, move_before_id: nil, group_full_path: nil)
+    def reorder_issue(issue, move_after_id: nil, move_before_id: nil)
       put :reorder,
            params: {
                namespace_id: project.namespace.to_param,
                project_id: project,
                id: issue.iid,
                move_after_id: move_after_id,
-               move_before_id: move_before_id,
-               group_full_path: group_full_path
+               move_before_id: move_before_id
            },
            format: :json
     end

spec/requests/api/issues/issues_spec.rb

@@ -1184,14 +1184,15 @@ RSpec.describe API::Issues do
   end
 
   describe 'PUT /projects/:id/issues/:issue_iid/reorder' do
-    let_it_be(:project) { create(:project) }
+    let_it_be(:group) { create(:group) }
+    let_it_be(:project) { create(:project, group: group) }
     let_it_be(:issue1) { create(:issue, project: project, relative_position: 10) }
     let_it_be(:issue2) { create(:issue, project: project, relative_position: 20) }
     let_it_be(:issue3) { create(:issue, project: project, relative_position: 30) }
 
     context 'when user has access' do
-      before do
-        project.add_developer(user)
+      before_all do
+        group.add_developer(user)
       end
 
       context 'with valid params' do
@@ -1217,6 +1218,19 @@ RSpec.describe API::Issues do
           expect(response).to have_gitlab_http_status(:not_found)
         end
       end
+
+      context 'with issue in different project' do
+        let(:other_project) { create(:project, group: group) }
+        let(:other_issue) { create(:issue, project: other_project, relative_position: 80) }
+
+        it 'reorders issues and returns a successful 200 response' do
+          put api("/projects/#{other_project.id}/issues/#{other_issue.iid}/reorder", user), params: { move_after_id: issue2.id, move_before_id: issue3.id }
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(other_issue.reload.relative_position)
+                .to be_between(issue2.reload.relative_position, issue3.reload.relative_position)
+        end
+      end
     end
 
     context 'with unauthorized user' do

spec/services/issues/reorder_service_spec.rb

@@ -67,20 +67,10 @@ RSpec.describe Issues::ReorderService do
       it_behaves_like 'issues reorder service'
 
       context 'when ordering in a group issue list' do
-        let(:params) { { move_after_id: issue2.id, move_before_id: issue3.id, group_full_path: group.full_path } }
+        let(:params) { { move_after_id: issue2.id, move_before_id: issue3.id } }
 
         subject { service(params) }
 
-        it 'sends the board_group_id parameter' do
-          match_params = { move_between_ids: [issue2.id, issue3.id], board_group_id: group.id }
-
-          expect(Issues::UpdateService)
-            .to receive(:new).with(project: project, current_user: user, params: match_params)
-            .and_return(double(execute: build(:issue)))
-
-          subject.execute(issue1)
-        end
-
         it 'sorts issues' do
           project2 = create(:project, namespace: group)
           issue4   = create(:issue, project: project2)

spec/services/issues/update_service_spec.rb

@@ -385,7 +385,6 @@ RSpec.describe Issues::UpdateService, :mailer do
           [issue_1, issue_2, issue_3].map(&:save)
 
           opts[:move_between_ids] = [issue_1.id, issue_2.id]
-          opts[:board_group_id] = group.id
 
           described_class.new(project: issue_3.project, current_user: user, params: opts).execute(issue_3)
           expect(issue_2.relative_position).to be_between(issue_1.relative_position, issue_2.relative_position)
@@ -1309,19 +1308,12 @@ RSpec.describe Issues::UpdateService, :mailer do
     end
 
     context 'when moving an issue ' do
-      it 'raises an error for invalid move ids within a project' do
+      it 'raises an error for invalid move ids' do
         opts = { move_between_ids: [9000, non_existing_record_id] }
 
         expect { described_class.new(project: issue.project, current_user: user, params: opts).execute(issue) }
             .to raise_error(ActiveRecord::RecordNotFound)
       end
-
-      it 'raises an error for invalid move ids within a group' do
-        opts = { move_between_ids: [9000, non_existing_record_id], board_group_id: create(:group).id }
-
-        expect { described_class.new(project: issue.project, current_user: user, params: opts).execute(issue) }
-            .to raise_error(ActiveRecord::RecordNotFound)
-      end
     end
 
     include_examples 'issuable update service' do

spec/support/shared_examples/services/boards/issues_move_service_shared_examples.rb

@@ -140,19 +140,6 @@ RSpec.shared_examples 'issues move service' do |group|
       expect(issue2.reload.updated_at.change(usec: 0)).to eq updated_at2.change(usec: 0)
     end
 
-    if group
-      context 'when on a group board' do
-        it 'sends the board_group_id parameter' do
-          params.merge!(move_after_id: issue1.id, move_before_id: issue2.id)
-
-          match_params = { move_between_ids: [issue1.id, issue2.id], board_group_id: parent.id }
-          expect(Issues::UpdateService).to receive(:new).with(project: issue.project, current_user: user, params: match_params).and_return(double(execute: build(:issue)))
-
-          described_class.new(parent, user, params).execute(issue)
-        end
-      end
-    end
-
     def reorder_issues(params, issues: [])
       issues.each do |issue|
         issue.move_to_end && issue.save!