Refactor uploads rewriter used when moving issue

app/services/issues/move_service.rb

@@ -62,26 +62,15 @@ module Issues
     end
     def rewrite_content(content)
-      rewrite_uploads(
-        unfold_references(content)
-      )
-    end
-
-    def unfold_references(content)
       return unless content
-      rewriter = Gitlab::Gfm::ReferenceRewriter.new(content, @old_project,
-                                                    @current_user)
-      rewriter.rewrite(@new_project)
-    end
-
-    def rewrite_uploads(content)
-      return unless content
-      rewriter = Gitlab::Gfm::UploadsRewriter.new(content, @old_project,
-                                                  @current_user)
-      return content unless rewriter.has_uploads?
-      rewriter.rewrite(@new_project)
+      rewriters = [Gitlab::Gfm::ReferenceRewriter,
+                   Gitlab::Gfm::UploadsRewriter]
+      rewriters.inject(content) do |text, klass|
+        rewriter = klass.new(text, @old_project, @current_user)
+        rewriter.needs_rewrite? ? rewriter.rewrite(@new_project) : text
+      end
     end
     def close_issue

lib/gitlab/gfm/reference_rewriter.rb

@@ -34,16 +34,19 @@ module Gitlab
         @source_project = source_project
         @current_user = current_user
         @original_html = markdown(text)
+        @pattern = Gitlab::ReferenceExtractor.references_pattern
       end
       def rewrite(target_project)
-        pattern = Gitlab::ReferenceExtractor.references_pattern
-
-        @text.gsub(pattern) do |reference|
+        @text.gsub(@pattern) do |reference|
           unfold_reference(reference, Regexp.last_match, target_project)
         end
       end
+      def needs_rewrite?
+        !(@text =~ @pattern).nil?
+      end
+
       private
       def unfold_reference(reference, match, target_project)

lib/gitlab/gfm/uploads_rewriter.rb

@@ -3,8 +3,9 @@ module Gitlab
     ##
     # Class that rewrites markdown links for uploads
     #
-    # Using a pattern defined in `FileUploader` copies files to a new project
-    # and rewrites all links to uploads in ain a given text.
+    # Using a pattern defined in `FileUploader` it copies files to a new
+    # project and rewrites all links to uploads in in a given text.
+    #
     #
     class UploadsRewriter
       def initialize(text, source_project, _current_user)
@@ -17,17 +18,17 @@ module Gitlab
         return unless @text
         new_uploader = file_uploader(target_project)
-        @text.gsub(@pattern) do |markdown_link|
-          old_file = find_file(@source_project, $~[:secret], $~[:file])
-          return markdown_link unless old_file.exists?
-          new_uploader.store!(old_file)
+        @text.gsub(@pattern) do |markdown|
+          file = find_file(@source_project, $~[:secret], $~[:file])
+          return markdown unless file.try(:exists?)
+          new_uploader.store!(file)
           new_uploader.to_h[:markdown]
         end
       end
-      def has_uploads?
-        !(@text =~ @pattern).nil?
+      def needs_rewrite?
+        files.any?
       end
       def files
@@ -46,8 +47,8 @@ module Gitlab
         uploader.file
       end
-      def file_uploader(*args)
-        uploader = FileUploader.new(*args)
+      def file_uploader(project, secret = nil)
+        uploader = FileUploader.new(project, secret)
         uploader.define_singleton_method(:move_to_store) { false }
         uploader
       end

spec/lib/gitlab/gfm/uploads_rewriter_spec.rb

@@ -33,8 +33,8 @@ describe Gitlab::Gfm::UploadsRewriter do
       end
     end
-    describe '#has_uploads?' do
-      subject { rewriter.has_uploads? }
+    describe '#needs_rewrite?' do
+      subject { rewriter.needs_rewrite? }
       it { is_expected.to eq true }
     end

spec/services/issues/move_service_spec.rb

@@ -162,12 +162,9 @@ describe Issues::MoveService, services: true do
         end
         context 'issue description with uploads' do
-          let(:path) { Rails.root + 'spec/fixtures/rails_sample.jpg' }
-          let(:file) { fixture_file_upload(path, 'image/jpg') }
-          let(:uploader) { FileUploader.new(old_project) }
-          let!(:store) { uploader.store!(file) }
+          let(:uploader) { build(:file_uploader, project: old_project) }
           let(:markdown) { uploader.to_h[:markdown] }
-          let(:description) { "Text and #{markdown}"}
+          let(:description) { "Text and #{markdown}" }
           include_context 'issue move executed'
@@ -176,8 +173,6 @@ describe Issues::MoveService, services: true do
             expect(new_issue.description)
               .to match(/Text and #{FileUploader::MARKDOWN_PATTERN}/)
           end
-
-          after { uploader.remove! }
         end
       end