Improvements in issue move feaure (refactoring)

According to endbosses' suggestions.

app/controllers/projects/issues_controller.rb

@@ -88,11 +88,11 @@ class Projects::IssuesController < Projects::ApplicationController
 
   def update
     @issue = Issues::UpdateService.new(project, current_user, issue_params).execute(issue)
-    move_service = Issues::MoveService.new(project, current_user, issue_params,
-                                           @issue, params['move_to_project_id'])
 
-    if move_service.move?
-      @issue = move_service.execute
+    if params[:move_to_project_id].to_i > 0
+      new_project = Project.find(params[:move_to_project_id])
+      move_service = Issues::MoveService.new(project, current_user, issue_params)
+      @issue = move_service.execute(@issue, new_project)
     end
 
     respond_to do |format|

app/models/concerns/issuable.rb

@@ -209,4 +209,13 @@ module Issuable
     Taskable.get_updated_tasks(old_content: previous_changes['description'].first,
                                new_content: description)
   end
+
+  ##
+  # Method that checks if issuable can be moved to another project.
+  #
+  # Should be overridden if issuable can be moved.
+  #
+  def can_move?(*)
+    false
+  end
 end

app/services/issues/create_service.rb

 module Issues
   class CreateService < Issues::BaseService
-    def execute(set_author: true)
+    def execute
       filter_params
       label_params = params[:label_ids]
       issue = project.issues.new(params.except(:label_ids))
-      issue.author = current_user if set_author
+      issue.author = params[:author] || current_user
 
       if issue.save
         issue.update_attributes(label_ids: label_params)

app/services/issues/move_service.rb

 module Issues
   class MoveService < Issues::BaseService
-    def initialize(project, current_user, params, issue, new_project_id)
-      super(project, current_user, params)
+    class MoveError < StandardError; end
 
-      @issue_old = issue
-      @issue_new = nil
-      @project_old = @project
+    def execute(issue, new_project)
+      @old_issue = issue
+      @old_project = @project
+      @new_project = new_project
 
-      if new_project_id.to_i > 0
-        @project_new = Project.find(new_project_id)
+      unless issue.can_move?(current_user, new_project)
+        raise MoveError, 'Cannot move issue due to insufficient permissions!'
       end
 
-      if @project_new == @project_old
-        raise StandardError, 'Cannot move issue to project it originates from!'
+      if @project == new_project
+        raise MoveError, 'Cannot move issue to project it originates from!'
       end
-    end
-
-    def execute
-      return unless move?
 
       # Using transaction because of a high resources footprint
       # on rewriting notes (unfolding references)
@@ -25,81 +21,72 @@ module Issues
       ActiveRecord::Base.transaction do
         # New issue tasks
         #
-        create_new_issue
+        @new_issue = create_new_issue
+
         rewrite_notes
-        add_moved_from_note
+        add_note_moved_from
 
         # Old issue tasks
         #
-        add_moved_to_note
-        close_old_issue
+        add_note_moved_to
+        close_issue
         mark_as_moved
       end
 
       notify_participants
 
-      @issue_new
-    end
-
-    def move?
-      @project_new && can_move?
+      @new_issue
     end
 
     private
 
-    def can_move?
-      @issue_old.can_move?(@current_user) &&
-        @issue_old.can_move?(@current_user, @project_new)
-    end
-
     def create_new_issue
-      new_params = { id: nil, iid: nil, milestone: nil, label_ids: [],
-                     project: @project_new, author: @issue_old.author,
-                     description: unfold_references(@issue_old.description) }
-
-      new_params = @issue_old.serializable_hash.merge(new_params)
-      create_service = CreateService.new(@project_new, @current_user,
-                                         new_params)
+      new_params = { id: nil, iid: nil, label_ids: [], milestone: nil,
+                     project: @new_project, author: @old_issue.author,
+                     description: unfold_references(@old_issue.description) }
 
-      @issue_new = create_service.execute(set_author: false)
+      new_params = @old_issue.serializable_hash.merge(new_params)
+      CreateService.new(@new_project, @current_user, new_params).execute
     end
 
     def rewrite_notes
-      @issue_old.notes.find_each do |note|
+      @old_issue.notes.find_each do |note|
         new_note = note.dup
-        new_params = { project: @project_new, noteable: @issue_new,
+        new_params = { project: @new_project, noteable: @new_issue,
                        note: unfold_references(new_note.note) }
 
         new_note.update(new_params)
       end
     end
 
-    def close_old_issue
-      close_service = CloseService.new(@project_new, @current_user)
-      close_service.execute(@issue_old, notifications: false, system_note: false)
+    def close_issue
+      close_service = CloseService.new(@old_project, @current_user)
+      close_service.execute(@old_issue, notifications: false, system_note: false)
     end
 
-    def add_moved_from_note
-      SystemNoteService.noteable_moved(@issue_new, @project_new,
-                                       @issue_old, @current_user, direction: :from)
+    def add_note_moved_from
+      SystemNoteService.noteable_moved(@new_issue, @new_project,
+                                       @old_issue, @current_user,
+                                       direction: :from)
     end
 
-    def add_moved_to_note
-      SystemNoteService.noteable_moved(@issue_old, @project_old,
-                                       @issue_new, @current_user, direction: :to)
+    def add_note_moved_to
+      SystemNoteService.noteable_moved(@old_issue, @old_project,
+                                       @new_issue, @current_user,
+                                       direction: :to)
     end
 
     def unfold_references(content)
-      unfolder = Gitlab::Gfm::ReferenceUnfolder.new(content, @project_old)
-      unfolder.unfold(@project_new)
+      unfolder = Gitlab::Gfm::ReferenceUnfolder.new(content, @old_project)
+      unfolder.unfold(@new_project)
     end
 
     def notify_participants
-      notification_service.issue_moved(@issue_old, @issue_new, @current_user)
+      notification_service.issue_moved(@old_issue, @new_issue, @current_user)
     end
 
     def mark_as_moved
-      @issue_old.update(moved_to: @issue_new)
+      @old_issue.update(moved_to: @new_issue)
     end
   end
 end

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) && issuable.can_move?(current_user)
+- if issuable.can_move?(current_user)
   %hr
   .form-group
     = label_tag :move_to_project_id, 'Move', class: 'control-label'

db/migrate/20160317092222_add_moved_to_to_issue.rb

 class AddMovedToToIssue < ActiveRecord::Migration
   def change
-    add_reference :issues, :moved_to, references: :issues, index: true
+    add_reference :issues, :moved_to, references: :issues
   end
 end

db/schema.rb

@@ -425,7 +425,6 @@ ActiveRecord::Schema.define(version: 20160317092222) do
   add_index "issues", ["created_at"], name: "index_issues_on_created_at", using: :btree
   add_index "issues", ["description"], name: "index_issues_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"}
   add_index "issues", ["milestone_id"], name: "index_issues_on_milestone_id", using: :btree
-  add_index "issues", ["moved_to_id"], name: "index_issues_on_moved_to_id", using: :btree
   add_index "issues", ["project_id", "iid"], name: "index_issues_on_project_id_and_iid", unique: true, using: :btree
   add_index "issues", ["project_id"], name: "index_issues_on_project_id", using: :btree
   add_index "issues", ["state"], name: "index_issues_on_state", using: :btree

lib/gitlab/gfm/reference_unfolder.rb

@@ -7,7 +7,9 @@ module Gitlab
     # in context of.
     #
     # `unfold` method tries to find all local references and unfold each of
-    # those local references to cross reference format.
+    # those local references to cross reference format, assuming that the
+    # argument passed to this method is a project that references will be
+    # viewed from (see `Referable#to_reference method).
     #
     # Examples:
     #
@@ -34,17 +36,12 @@ module Gitlab
       end
 
       def unfold(from_project)
-        return @text unless @text =~ references_pattern
+        pattern = Gitlab::ReferenceExtractor.references_pattern
+        return @text unless @text =~ pattern
 
-        unfolded = @text.gsub(references_pattern) do |reference|
+        @text.gsub(pattern) do |reference|
           unfold_reference(reference, Regexp.last_match, from_project)
         end
-
-        unless substitution_valid?(unfolded)
-          raise StandardError, 'Invalid references unfolding!'
-        end
-
-        unfolded
       end
 
       private
@@ -61,16 +58,6 @@ module Gitlab
         substitution_valid?(new_text) ? cross_reference : reference
       end
 
-      def references_pattern
-        return @pattern if @pattern
-
-        patterns = Gitlab::ReferenceExtractor::REFERABLES.map do |ref|
-          ref.to_s.classify.constantize.try(:reference_pattern)
-        end
-
-        @pattern = Regexp.union(patterns.compact)
-      end
-
       def referables
         return @referables if @referables
 

lib/gitlab/reference_extractor.rb

@@ -37,6 +37,16 @@ module Gitlab
       @references.values.flatten
     end
 
+    def self.references_pattern
+      return @pattern if @pattern
+
+      patterns = REFERABLES.map do |ref|
+        ref.to_s.classify.constantize.try(:reference_pattern)
+      end
+
+      @pattern = Regexp.union(patterns.compact)
+    end
+
     private
 
     def reference_context

spec/lib/gitlab/reference_extractor_spec.rb

@@ -134,4 +134,9 @@ describe Gitlab::ReferenceExtractor, lib: true do
       expect(subject.all).to match_array([issue, label])
     end
   end
+
+  describe '.references_pattern' do
+    subject { described_class.references_pattern }
+    it { is_expected.to be_kind_of Regexp }
+  end
 end

spec/services/issues/move_service_spec.rb

@@ -15,11 +15,7 @@ describe Issues::MoveService, services: true do
   end
 
   let(:move_service) do
-    described_class.new(old_project, user, issue_params, old_issue, new_project_id)
-  end
-
-  shared_context 'issue move requested' do
-    let(:new_project_id) { new_project.id }
+    described_class.new(old_project, user, issue_params)
   end
 
   shared_context 'user can move issue' do
@@ -29,20 +25,14 @@ describe Issues::MoveService, services: true do
     end
   end
 
-  context 'issue movable' do
-    include_context 'issue move requested'
-    include_context 'user can move issue'
-
-    describe '#move?' do
-      subject { move_service.move? }
-      it { is_expected.to be_truthy }
-    end
-
   describe '#execute' do
     shared_context 'issue move executed' do
-        let!(:new_issue) { move_service.execute }
+      let!(:new_issue) { move_service.execute(old_issue, new_project) }
     end
 
+    context 'issue movable' do
+      include_context 'user can move issue'
+
       context 'generic issue' do
         include_context 'issue move executed'
 
@@ -164,57 +154,33 @@ describe Issues::MoveService, services: true do
           end
         end
       end
-    end
-  end
 
       context 'moving to same project' do
         let(:new_project) { old_project }
 
-    include_context 'issue move requested'
-    include_context 'user can move issue'
-
         it 'raises error' do
-      expect { move_service }
+          expect { move_service.execute(old_issue, new_project) }
             .to raise_error(StandardError, /Cannot move issue/)
         end
       end
-
-  context 'issue move not requested' do
-    let(:new_project_id) { nil }
-
-    describe '#move?' do
-      subject { move_service.move? }
-
-      context 'user do not have permissions to move issue' do
-        it { is_expected.to be_falsey }
-      end
-
-      context 'user has permissions to move issue' do
-        include_context 'user can move issue'
-        it { is_expected.to be_falsey }
-      end
-    end
     end
 
     describe 'move permissions' do
-    include_context 'issue move requested'
-
-    describe '#move?' do
-      subject { move_service.move? }
+      let(:move) { move_service.execute(old_issue, new_project) }
 
       context 'user is reporter in both projects' do
         include_context 'user can move issue'
-        it { is_expected.to be_truthy }
+        it { expect { move }.to_not raise_error }
       end
 
       context 'user is reporter only in new project' do
         before { new_project.team << [user, :reporter] }
-        it { is_expected.to be_falsey }
+        it { expect { move }.to raise_error(StandardError, /permissions/) }
       end
 
       context 'user is reporter only in old project' do
         before { old_project.team << [user, :reporter] }
-        it { is_expected.to be_falsey }
+        it { expect { move }.to raise_error(StandardError, /permissions/) }
       end
 
       context 'user is reporter in one project and guest in another' do
@@ -223,7 +189,7 @@ describe Issues::MoveService, services: true do
           old_project.team << [user, :reporter]
         end
 
-        it { is_expected.to be_falsey }
+        it { expect { move }.to raise_error(StandardError, /permissions/) }
       end
 
       context 'issue has already been moved' do
@@ -236,7 +202,7 @@ describe Issues::MoveService, services: true do
                          moved_to: moved_to_issue)
         end
 
-        it { is_expected.to be_falsey }
+        it { expect { move }.to raise_error(StandardError, /permissions/) }
       end
     end
   end