Support use of group_full_path when reorderingtesting-manual-ordering

which scopes the issues to a group and subgroups

5 files changed, 26 insertions, 8 deletions

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 48f82a64818..f221f0363d3 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -254,7 +254,7 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def reorder_params
-    params.permit(:move_before_id, :move_after_id)
+    params.permit(:move_before_id, :move_after_id, :group_full_path)
   end
 
   def store_uri
diff --git a/app/services/issues/reorder_service.rb b/app/services/issues/reorder_service.rb
index 1013c3dc20b..7b8cc36607d 100644
--- a/app/services/issues/reorder_service.rb
+++ b/app/services/issues/reorder_service.rb
@@ -2,8 +2,9 @@
 
 module Issues
   class ReorderService < Issues::BaseService
-    def execute(issue, group = nil)
+    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?
@@ -13,6 +14,12 @@ 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, current_user, attrs).execute(issue)
     rescue ActiveRecord::RecordNotFound
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index cb2337d29d4..6b9f23f24cd 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -76,6 +76,7 @@ module Issues
 
       issue_before = get_issue_if_allowed(before_id, board_group_id)
       issue_after = get_issue_if_allowed(after_id, board_group_id)
+      raise ActiveRecord::RecordNotFound unless issue_before || issue_after
 
       issue.move_between(issue_before, issue_after)
     end
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 743c1cd37ad..b64e55b5255 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -321,6 +321,7 @@ describe Projects::IssuesController do
   end
 
   describe 'PUT #reorder' do
+    let(:group)  { create(:group, projects: [project]) }
     let(:issue1) { create(:issue, project: project) }
     let(:issue2) { create(:issue, project: project) }
     let(:issue3) { create(:issue, project: project) }
@@ -340,7 +341,7 @@ describe Projects::IssuesController do
             issue.move_to_end && issue.save!
           end
 
-          reorder_issue issue1, move_after_id: issue2.id, move_before_id: issue3.id
+          reorder_issue issue1, move_after_id: issue2.id, move_before_id: issue3.id, group_full_path: group.full_path
 
           [issue1, issue2, issue3].each { |issue| issue.reload }
 
@@ -361,6 +362,14 @@ describe Projects::IssuesController do
 
           expect(response).to have_gitlab_http_status(404)
         end
+
+        it 'returns a unprocessable entity 422 response for invalid move ids' do
+          another_group = create(:group)
+
+          reorder_issue issue1, move_after_id: issue2.id, move_before_id: issue3.id, group_full_path: another_group.full_path
+
+          expect(response).to have_gitlab_http_status(422)
+        end
       end
     end
 
@@ -376,14 +385,15 @@ describe Projects::IssuesController do
       end
     end
 
-    def reorder_issue(issue, move_after_id: nil, move_before_id: nil)
+    def reorder_issue(issue, move_after_id: nil, move_before_id: nil, group_full_path: 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
+               move_before_id: move_before_id,
+               group_full_path: group_full_path
            },
            format: :json
     end
diff --git a/spec/services/issues/reorder_service_spec.rb b/spec/services/issues/reorder_service_spec.rb
index 4b481f20061..7ca6db002ed 100644
--- a/spec/services/issues/reorder_service_spec.rb
+++ b/spec/services/issues/reorder_service_spec.rb
@@ -28,12 +28,12 @@ describe Issues::ReorderService do
       if use_group
         context 'when ordering in a group issue list' do
           it 'sends the board_group_id parameter' do
-            params = { move_after_id: issue2.id, move_before_id: issue3.id }
+            params = { move_after_id: issue2.id, move_before_id: issue3.id, group_full_path: group.full_path }
 
             match_params = { move_between_ids: [issue2.id, issue3.id], board_group_id: group.id }
             expect(Issues::UpdateService).to receive(:new).with(project, user, match_params).and_return(double(execute: build(:issue)))
 
-            described_class.new(project, user, params).execute(issue1, group)
+            described_class.new(project, user, params).execute(issue1)
           end
 
           it 'sorts issues' do
@@ -44,7 +44,7 @@ describe Issues::ReorderService do
               issue.move_to_end && issue.save!
             end
 
-            params = { move_after_id: issue2.id, move_before_id: issue3.id }
+            params = { move_after_id: issue2.id, move_before_id: issue3.id, group_full_path: group.full_path }
 
             described_class.new(project, user, params).execute(issue4)