Merge branch '341562-fix-issue-card-position-on-create' into 'master'

Refactor issue move to not require group_id param

See merge request gitlab-org/gitlab!79309

app/controllers/projects/issues_controller.rb

@@ -321,7 +321,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,11 @@ 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)
+    positioning_scope = issuable_position.class.relative_positioning_query_base(issuable_position)
+
+    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

@@ -2,47 +2,31 @@
 
 module Issues
   class ReorderService < Issues::BaseService
+    include Gitlab::Utils::StrongMemoize
+
     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)
-      return false if attrs.empty?
+      return false unless move_between_ids
 
-      update(issue, attrs)
+      update(issue, { move_between_ids: move_between_ids })
     end
 
     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)
-      attrs = {}
-
-      if move_between_ids
-        attrs[:move_between_ids] = move_between_ids
-        attrs[:board_group_id]   = group&.id
-      end
-
-      attrs
-    end
-
     def move_between_ids
-      ids = [params[:move_after_id], params[:move_before_id]]
-              .map(&:to_i)
-              .map { |m| m > 0 ? m : nil }
+      strong_memoize(:move_between_ids) do
+        ids = [params[:move_after_id], params[:move_before_id]]
+                .map(&:to_i)
+                .map { |m| m > 0 ? m : nil }
 
-      ids.any? ? ids : nil
+        ids.any? ? ids : nil
+      end
     end
   end
 end

app/services/issues/update_service.rb

@@ -100,10 +100,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 +217,13 @@ 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 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

@@ -2,6 +2,8 @@
 
 module Epics
   class UpdateService < Epics::BaseService
+    include Gitlab::Utils::StrongMemoize
+
     EPIC_DATE_FIELDS = %I[
       start_date_fixed
       start_date_is_fixed
@@ -9,6 +11,8 @@ module Epics
       due_date_is_fixed
     ].freeze
 
+    attr_reader :epic_board_id
+
     def execute(epic)
       reposition_on_board(epic)
 
@@ -104,27 +108,25 @@ module Epics
     end
 
     def reposition_on_board(epic)
+      @epic_board_id = params.delete(:board_id)
+
       return unless params[:move_between_ids]
       return unless epic_board_id
 
       fill_missing_positions_before
 
-      epic_board_position = issuable_for_positioning(epic.id, epic_board_id, create_missing: true)
+      # we want to create missing only for the epic being moved
+      # other records are handled by PositionCreateService
+      epic_board_position = Boards::EpicBoardPosition.find_or_create_by!(epic_board_id: epic_board_id, epic_id: epic.id) # rubocop: disable CodeReuse/ActiveRecord
       handle_move_between_ids(epic_board_position)
 
       epic_board_position.save!
     end
 
-    # we want to create missing only for the epic being moved
-    # other records are handled by PositionCreateService
-    def issuable_for_positioning(id, board_id, create_missing: false)
+    def issuable_for_positioning(id, positioning_scope)
       return unless id
 
-      position = Boards::EpicBoardPosition.find_by_epic_id_and_epic_board_id(id, board_id)
-
-      return position if position
-
-      Boards::EpicBoardPosition.create!(epic_id: id, epic_board_id: board_id) if create_missing
+      positioning_scope.find_by_epic_id(id)
     end
 
     def fill_missing_positions_before
@@ -134,7 +136,7 @@ module Epics
 
       return unless before_id
       # if position for the epic above exists we don't need to create positioning records
-      return if issuable_for_positioning(before_id, epic_board_id)
+      return if Boards::EpicBoardPosition.exists?(epic_board_id: epic_board_id, epic_id: before_id) # rubocop: disable CodeReuse/ActiveRecord
 
       service_params = {
         board_id: epic_board_id,
@@ -145,14 +147,6 @@ module Epics
       Boards::Epics::PositionCreateService.new(board_group, current_user, service_params).execute
     end
 
-    def epic_board_id
-      params[positioning_scope_key]
-    end
-
-    def positioning_scope_key
-      :board_id
-    end
-
     def saved_change_to_epic_dates?(epic)
       (epic.saved_changes.keys.map(&:to_sym) & EPIC_DATE_FIELDS).present?
     end

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!