Prevent issue move if issue has been already moved

6 files changed, 99 insertions, 3 deletions

diff --git a/app/models/issue.rb b/app/models/issue.rb
index b506d174155..6a016636e0d 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -123,4 +123,16 @@ class Issue < ActiveRecord::Base
       note.all_references(current_user).merge_requests
     end.uniq.select { |mr| mr.open? && mr.closes_issue?(self) }
   end
+
+  def moved?
+    !moved_to.nil?
+  end
+
+  def can_move?(user, to_project = nil)
+    if to_project
+      return false unless user.can?(:admin_issue, to_project)
+    end
+
+    !moved? && user.can?(:admin_issue, self.project)
+  end
 end
diff --git a/app/services/issues/move_service.rb b/app/services/issues/move_service.rb
index ce31830f2d0..47b58d973f9 100644
--- a/app/services/issues/move_service.rb
+++ b/app/services/issues/move_service.rb
@@ -33,6 +33,7 @@ module Issues
         #
         add_moved_to_note
         close_old_issue
+        mark_as_moved
       end
 
       notify_participants
@@ -47,8 +48,8 @@ module Issues
     private
 
     def can_move?
-      can?(@current_user, :admin_issue, @project_old) &&
-        can?(@current_user, :admin_issue, @project_new)
+      @issue_old.can_move?(@current_user) &&
+        @issue_old.can_move?(@current_user, @project_new)
     end
 
     def create_new_issue
@@ -96,5 +97,9 @@ module Issues
     def notify_participants
       notification_service.issue_moved(@issue_old, @issue_new, @current_user)
     end
+
+    def mark_as_moved
+      @issue_old.update(moved_to: @issue_new)
+    end
   end
 end
diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml
index d3eabe9ea64..5276afec1ca 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/app/views/shared/issuable/_form.html.haml
@@ -67,7 +67,7 @@
       - if can? current_user, :admin_label, issuable.project
         = link_to 'Create new label', new_namespace_project_label_path(issuable.project.namespace, issuable.project), target: :blank
 
-- if issuable.is_a?(Issue) && can?(current_user, :admin_issue, issuable.project)
+- if issuable.is_a?(Issue) && issuable.can_move?(current_user)
   %hr
     .form-group
       = label_tag :move_to_project_id, 'Move', class: 'control-label'
diff --git a/spec/features/issues/move_spec.rb b/spec/features/issues/move_spec.rb
index 6303b117c09..db26b3a4671 100644
--- a/spec/features/issues/move_spec.rb
+++ b/spec/features/issues/move_spec.rb
@@ -60,6 +60,17 @@ feature 'issue move to another project' do
         expect(page).to have_select('move_to_project_id', options: options)
       end
     end
+
+    context 'issue has been already moved' do
+      let(:new_issue) { create(:issue, project: new_project) }
+      let(:issue) do
+        create(:issue, project: old_project, author: user, moved_to: new_issue)
+      end
+
+      scenario 'user wants to move issue that has already been moved' do
+        expect(page).to have_no_select('move_to_project_id')
+      end
+    end
   end
 
   def edit_issue(issue)
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 7f44ca2f7db..0d6e9cb3a4c 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -130,6 +130,56 @@ describe Issue, models: true do
     end
   end
 
+  describe '#can_move?' do
+    let(:user) { create(:user) }
+    let(:issue) { create(:issue) }
+    subject { issue.can_move?(user) }
+
+    context 'user is not a member of project issue belongs to' do
+      it { is_expected.to eq false}
+    end
+
+    context 'user is reporter in project issue belongs to' do
+      let(:project) { create(:project) }
+      let(:issue) { create(:issue, project: project) }
+
+      before { project.team << [user, :reporter] }
+
+      it { is_expected.to eq true }
+
+      context 'checking destination project also' do
+        subject { issue.can_move?(user, to_project) }
+        let(:to_project) { create(:project) }
+
+        context 'destination project allowed' do
+          before { to_project.team << [user, :reporter] }
+          it { is_expected.to eq true }
+        end
+
+        context 'destination project not allowed' do
+          before { to_project.team << [user, :guest] }
+          it { is_expected.to eq false }
+        end
+      end
+    end
+  end
+
+  describe '#moved?' do
+    let(:issue) { create(:issue) }
+    subject { issue.moved? }
+
+    context 'issue not moved' do
+      it { is_expected.to eq false }
+    end
+
+    context 'issue already moved' do
+      let(:moved_to_issue) { create(:issue) }
+      let(:issue) { create(:issue, moved_to: moved_to_issue) }
+
+      it { is_expected.to eq true }
+    end
+  end
+
   it_behaves_like 'an editable mentionable' do
     subject { create(:issue) }
 
diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb
index cb02b8721ac..02ade91ac03 100644
--- a/spec/services/issues/move_service_spec.rb
+++ b/spec/services/issues/move_service_spec.rb
@@ -91,6 +91,11 @@ describe Issues::MoveService, services: true do
         it 'creates a new internal id for issue' do
           expect(new_issue.iid).to be 1
         end
+
+        it 'marks issue as moved' do
+          expect(old_issue.moved?).to eq true
+          expect(old_issue.moved_to).to eq new_issue
+        end
       end
 
       context 'issue with notes' do
@@ -220,6 +225,19 @@ describe Issues::MoveService, services: true do
 
         it { is_expected.to be_falsey }
       end
+
+      context 'issue has already been moved' do
+        include_context 'user can move issue'
+
+        let(:moved_to_issue) { create(:issue) }
+
+        let(:old_issue) do
+          create(:issue, project: old_project, author: author,
+                         moved_to: moved_to_issue)
+        end
+
+        it { is_expected.to be_falsey }
+      end
     end
   end
 end