Merge branch 'fix/issue-move-rewrite-uploads' into 'master'

Rewrite uploads when moving issue to another project

Closes #14531

See merge request !3382
Signed-off-by: Rémy Coutable <remy@rymai.me>

CHANGELOG

@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 v 8.6.3 (unreleased)
   - Destroy related todos when an Issue/MR is deleted. !3376
   - Fix error 500 when target is nil on todo list. !3376
+  - Fix copying uploads when moving issue to another project. !3382
   - Fix raw/rendered diff producing different results on merge requests. !3450
   - Fix commit comment alignment (Stan Hu). !3466
   - Update gitlab-shell version and doc to 2.6.12. gitlab-org/gitlab-ee!280

app/services/issues/move_service.rb

@@ -43,7 +43,7 @@ module Issues
     def create_new_issue
       new_params = { id: nil, iid: nil, label_ids: [], milestone: nil,
                      project: @new_project, author: @old_issue.author,
-                     description: unfold_references(@old_issue.description) }
+                     description: rewrite_content(@old_issue.description) }
 
       new_params = @old_issue.serializable_hash.merge(new_params)
       CreateService.new(@new_project, @current_user, new_params).execute
@@ -53,13 +53,26 @@ module Issues
       @old_issue.notes.find_each do |note|
         new_note = note.dup
         new_params = { project: @new_project, noteable: @new_issue,
-                       note: unfold_references(new_note.note),
-                       created_at: note.created_at }
+                       note: rewrite_content(new_note.note),
+                       created_at: note.created_at,
+                       updated_at: note.updated_at }
 
         new_note.update(new_params)
       end
     end
 
+    def rewrite_content(content)
+      return unless content
+
+      rewriters = [Gitlab::Gfm::ReferenceRewriter,
+                   Gitlab::Gfm::UploadsRewriter]
+
+      rewriters.inject(content) do |text, klass|
+        rewriter = klass.new(text, @old_project, @current_user)
+        rewriter.rewrite(@new_project)
+      end
+    end
+
     def close_issue
       close_service = CloseService.new(@old_project, @current_user)
       close_service.execute(@old_issue, notifications: false, system_note: false)
@@ -77,20 +90,12 @@ module Issues
                                        direction: :to)
     end
 
-    def unfold_references(content)
-      return unless content
-
-      rewriter = Gitlab::Gfm::ReferenceRewriter.new(content, @old_project,
-                                                    @current_user)
-      rewriter.rewrite(@new_project)
+    def mark_as_moved
+      @old_issue.update(moved_to: @new_issue)
     end
 
     def notify_participants
       notification_service.issue_moved(@old_issue, @new_issue, @current_user)
     end
-
-    def mark_as_moved
-      @old_issue.update(moved_to: @new_issue)
-    end
   end
 end

app/uploaders/file_uploader.rb

 # encoding: utf-8
 class FileUploader < CarrierWave::Uploader::Base
   include UploaderHelper
+  MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}
 
   storage :file
 
   attr_accessor :project, :secret
 
-  def initialize(project, secret = self.class.generate_secret)
+  def initialize(project, secret = nil)
     @project = project
-    @secret = secret
+    @secret = secret || self.class.generate_secret
   end
 
   def base_dir
@@ -23,14 +24,14 @@ class FileUploader < CarrierWave::Uploader::Base
     File.join(base_dir, 'tmp', @project.path_with_namespace, @secret)
   end
 
-  def self.generate_secret
-    SecureRandom.hex
-  end
-
   def secure_url
     File.join("/uploads", @secret, file.filename)
   end
 
+  def to_markdown
+    to_h[:markdown]
+  end
+
   def to_h
     filename = image? ? self.file.basename : self.file.filename
     escaped_filename = filename.gsub("]", "\\]")
@@ -45,4 +46,8 @@ class FileUploader < CarrierWave::Uploader::Base
       markdown: markdown
     }
   end
+
+  def self.generate_secret
+    SecureRandom.hex
+  end
 end

lib/gitlab/gfm/reference_rewriter.rb

@@ -34,16 +34,21 @@ 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
+        return @text unless needs_rewrite?
 
-        @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
+      end
+
       private
 
       def unfold_reference(reference, match, target_project)

lib/gitlab/gfm/uploads_rewriter.rb

+module Gitlab
+  module Gfm
+    ##
+    # Class that rewrites markdown links for uploads
+    #
+    # 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)
+        @text = text
+        @source_project = source_project
+        @pattern = FileUploader::MARKDOWN_PATTERN
+      end
+
+      def rewrite(target_project)
+        return @text unless needs_rewrite?
+
+        @text.gsub(@pattern) do |markdown|
+          file = find_file(@source_project, $~[:secret], $~[:file])
+          return markdown unless file.try(:exists?)
+
+          new_uploader = FileUploader.new(target_project)
+          new_uploader.store!(file)
+          new_uploader.to_markdown
+        end
+      end
+
+      def needs_rewrite?
+        files.any?
+      end
+
+      def files
+        referenced_files = @text.scan(@pattern).map do
+          find_file(@source_project, $~[:secret], $~[:file])
+        end
+
+        referenced_files.compact.select(&:exists?)
+      end
+
+      private
+
+      def find_file(project, secret, file)
+        uploader = FileUploader.new(project, secret)
+        uploader.retrieve_from_store!(file)
+        uploader.file
+      end
+    end
+  end
+end

spec/factories/file_uploader.rb

+FactoryGirl.define do
+  factory :file_uploader do
+    project
+    secret nil
+
+    transient do
+      fixture { 'rails_sample.jpg' }
+      path { File.join(Rails.root, 'spec/fixtures', fixture) }
+      file { Rack::Test::UploadedFile.new(path) }
+    end
+
+    after(:build) do |uploader, evaluator|
+      uploader.store!(evaluator.file)
+    end
+
+    initialize_with do
+      new(project, secret)
+    end
+  end
+end

spec/factories_spec.rb

 require 'spec_helper'
 
-FactoryGirl.factories.map(&:name).each do |factory_name|
-  describe "#{factory_name} factory" do
-    it 'should be valid' do
-      expect(build(factory_name)).to be_valid
+describe 'factories' do
+  FactoryGirl.factories.each do |factory|
+    describe "#{factory.name} factory" do
+      let(:entity) { build(factory.name) }
+
+      it 'does not raise error when created 'do
+        expect { entity }.to_not raise_error
+      end
+
+      it 'should be valid', if: factory.build_class < ActiveRecord::Base do
+        expect(entity).to be_valid
+      end
     end
   end
 end

spec/lib/gitlab/gfm/uploads_rewriter_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Gfm::UploadsRewriter do
+  let(:user) { create(:user) }
+  let(:old_project) { create(:project) }
+  let(:new_project) { create(:project) }
+  let(:rewriter) { described_class.new(text, old_project, user) }
+
+  context 'text contains links to uploads' do
+    let(:image_uploader) do
+      build(:file_uploader, project: old_project)
+    end
+
+    let(:zip_uploader) do
+      build(:file_uploader, project: old_project,
+                            fixture: 'ci_build_artifacts.zip')
+    end
+
+    let(:text) do
+      "Text and #{image_uploader.to_markdown} and #{zip_uploader.to_markdown}"
+    end
+
+    describe '#rewrite' do
+      let!(:new_text) { rewriter.rewrite(new_project) }
+
+      let(:old_files) { [image_uploader, zip_uploader].map(&:file) }
+      let(:new_files) do
+        described_class.new(new_text, new_project, user).files
+      end
+
+      let(:old_paths) { old_files.map(&:path) }
+      let(:new_paths) { new_files.map(&:path) }
+
+      it 'rewrites content' do
+        expect(new_text).to_not eq text
+        expect(new_text.length).to eq text.length
+      end
+
+      it 'copies files' do
+        expect(new_files).to all(exist)
+        expect(old_paths).to_not match_array new_paths
+        expect(old_paths).to all(include(old_project.path_with_namespace))
+        expect(new_paths).to all(include(new_project.path_with_namespace))
+      end
+
+      it 'does not remove old files' do
+        expect(old_files).to all(exist)
+      end
+
+      it 'generates a new secret for each file' do
+        expect(new_paths).to_not include image_uploader.secret
+        expect(new_paths).to_not include zip_uploader.secret
+      end
+    end
+
+    describe '#needs_rewrite?' do
+      subject { rewriter.needs_rewrite? }
+      it { is_expected.to eq true }
+    end
+
+    describe '#files' do
+      subject { rewriter.files }
+      it { is_expected.to be_an(Array) }
+    end
+  end
+end

spec/services/issues/move_service_spec.rb

@@ -143,6 +143,20 @@ describe Issues::MoveService, services: true do
               .to eq "Note with reference to merge request #{old_project.to_reference}!1"
           end
         end
+
+        context 'issue description with uploads' do
+          let(:uploader) { build(:file_uploader, project: old_project) }
+          let(:description) { "Text and #{uploader.to_markdown}" }
+
+          include_context 'issue move executed'
+
+          it 'rewrites uploads in description' do
+            expect(new_issue.description).to_not eq description
+            expect(new_issue.description)
+              .to match(/Text and #{FileUploader::MARKDOWN_PATTERN}/)
+            expect(new_issue.description).to_not include uploader.secret
+          end
+        end
       end
 
       describe 'rewritting references' do