Merge branch 'bw-issue-reorder' into 'master'

Add ability to reorder issues

See merge request gitlab-org/gitlab-ce!29012

8 files changed, 257 insertions, 3 deletions

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index b4d89db20c5..b16f3dd9d82 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -33,7 +33,7 @@ class Projects::IssuesController < Projects::ApplicationController
   before_action :authorize_create_issue!, only: [:new, :create]
 
   # Allow modify issue
-  before_action :authorize_update_issuable!, only: [:edit, :update, :move]
+  before_action :authorize_update_issuable!, only: [:edit, :update, :move, :reorder]
 
   # Allow create a new branch and empty WIP merge request from current issue
   before_action :authorize_create_merge_request_from!, only: [:create_merge_request]
@@ -132,6 +132,16 @@ class Projects::IssuesController < Projects::ApplicationController
     render_conflict_response
   end
 
+  def reorder
+    service = Issues::ReorderService.new(project, current_user, reorder_params)
+
+    if service.execute(issue)
+      head :ok
+    else
+      head :unprocessable_entity
+    end
+  end
+
   def related_branches
     @related_branches = Issues::RelatedBranchesService.new(project, current_user).execute(issue)
 
@@ -239,6 +249,10 @@ class Projects::IssuesController < Projects::ApplicationController
     ] + [{ label_ids: [], assignee_ids: [], update_task: [:index, :checked, :line_number, :line_source] }]
   end
 
+  def reorder_params
+    params.permit(:move_before_id, :move_after_id, :group_full_path)
+  end
+
   def store_uri
     if request.get? && !request.xhr?
       store_location_for :user, request.fullpath
diff --git a/app/services/boards/issues/move_service.rb b/app/services/boards/issues/move_service.rb
index 834baeb9643..e27d34dbcab 100644
--- a/app/services/boards/issues/move_service.rb
+++ b/app/services/boards/issues/move_service.rb
@@ -79,9 +79,11 @@ module Boards
       # rubocop: enable CodeReuse/ActiveRecord
 
       def move_between_ids
-        return unless params[:move_after_id] || params[:move_before_id]
+        ids = [params[:move_after_id], params[:move_before_id]]
+                .map(&:to_i)
+                .map { |m| m.positive? ? m : nil }
 
-        [params[:move_after_id], params[:move_before_id]]
+        ids.any? ? ids : nil
       end
     end
   end
diff --git a/app/services/issues/reorder_service.rb b/app/services/issues/reorder_service.rb
new file mode 100644
index 00000000000..02c18d31b5e
--- /dev/null
+++ b/app/services/issues/reorder_service.rb
@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+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)
+      return false if attrs.empty?
+
+      update(issue, attrs)
+    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, current_user, 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.positive? ? m : nil }
+
+      ids.any? ? ids : nil
+    end
+  end
+end
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/config/routes/project.rb b/config/routes/project.rb
index a1e769f6ca3..0e8e089c78a 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -406,6 +406,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           post :toggle_subscription
           post :mark_as_spam
           post :move
+          put :reorder
           get :related_branches
           get :can_create_branch
           get :realtime_changes
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 32607fc5f56..f82e3c8c7dc 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -320,6 +320,90 @@ describe Projects::IssuesController do
     end
   end
 
+  describe 'PUT #reorder' do
+    let(:group)   { create(:group, projects: [project]) }
+    let!(:issue1) { create(:issue, project: project, relative_position: 10) }
+    let!(:issue2) { create(:issue, project: project, relative_position: 20) }
+    let!(:issue3) { create(:issue, project: project, relative_position: 30) }
+
+    before do
+      sign_in(user)
+    end
+
+    context 'when user has access' do
+      before do
+        project.add_developer(user)
+      end
+
+      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)
+
+          [issue1, issue2, issue3].map(&:reload)
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(issue1.relative_position)
+            .to be_between(issue2.relative_position, issue3.relative_position)
+        end
+      end
+
+      context 'with invalid params' do
+        it 'returns a unprocessable entity 422 response for invalid move ids' do
+          reorder_issue(issue1, move_after_id: 99, move_before_id: 999)
+
+          expect(response).to have_gitlab_http_status(422)
+        end
+
+        it 'returns a not found 404 response for invalid issue id' do
+          reorder_issue(object_double(issue1, iid: 999),
+            move_after_id: issue2.id,
+            move_before_id: issue3.id)
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+
+        it 'returns a unprocessable entity 422 response for issues not in group' 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
+
+    context 'with unauthorized user' do
+      before do
+        project.add_guest(user)
+      end
+
+      it 'responds with 404' do
+        reorder_issue(issue1, move_after_id: issue2.id, move_before_id: issue3.id)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    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,
+               group_full_path: group_full_path
+           },
+           format: :json
+    end
+  end
+
   describe 'PUT #update' do
     subject do
       put :update,
diff --git a/spec/services/issues/reorder_service_spec.rb b/spec/services/issues/reorder_service_spec.rb
new file mode 100644
index 00000000000..b147cdf4e64
--- /dev/null
+++ b/spec/services/issues/reorder_service_spec.rb
@@ -0,0 +1,88 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Issues::ReorderService do
+  set(:user)    { create(:user) }
+  set(:project) { create(:project) }
+  set(:group)   { create(:group) }
+
+  shared_examples 'issues reorder service' do
+    context 'when reordering issues' do
+      it 'returns false with no params' do
+        expect(service({}).execute(issue1)).to be_falsey
+      end
+
+      it 'returns false with both invalid params' do
+        params = { move_after_id: nil, move_before_id: 1 }
+
+        expect(service(params).execute(issue1)).to be_falsey
+      end
+
+      it 'sorts issues' do
+        params = { move_after_id: issue2.id, move_before_id: issue3.id }
+
+        service(params).execute(issue1)
+
+        expect(issue1.relative_position)
+          .to be_between(issue2.relative_position, issue3.relative_position)
+      end
+    end
+  end
+
+  describe '#execute' do
+    let(:issue1) { create(:issue, project: project, relative_position: 10) }
+    let(:issue2) { create(:issue, project: project, relative_position: 20) }
+    let(:issue3) { create(:issue, project: project, relative_position: 30) }
+
+    context 'when ordering issues in a project' do
+      let(:parent) { project }
+
+      before do
+        parent.add_developer(user)
+      end
+
+      it_behaves_like 'issues reorder service'
+    end
+
+    context 'when ordering issues in a group' do
+      let(:project) { create(:project, namespace: group) }
+
+      before do
+        group.add_developer(user)
+      end
+
+      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 } }
+
+        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, user, 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)
+
+          subject.execute(issue4)
+
+          expect(issue4.relative_position)
+            .to be_between(issue2.relative_position, issue3.relative_position)
+        end
+      end
+    end
+  end
+
+  def service(params)
+    described_class.new(project, user, params)
+  end
+end
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index 22f5607cb9c..28fa5d12d9c 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -687,6 +687,22 @@ describe Issues::UpdateService, :mailer do
       end
     end
 
+    context 'when moving an issue ', :nested_groups do
+      it 'raises an error for invalid move ids within a project' do
+        opts = { move_between_ids: [9000, 9999] }
+
+        expect { described_class.new(issue.project, user, 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, 9999], board_group_id: create(:group).id }
+
+        expect { described_class.new(issue.project, user, opts).execute(issue) }
+            .to raise_error(ActiveRecord::RecordNotFound)
+      end
+    end
+
     include_examples 'issuable update service' do
       let(:open_issuable) { issue }
       let(:closed_issuable) { create(:closed_issue, project: project) }