Commit 98df8aab authored by Douwe Maan's avatar Douwe Maan

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

Rewrite uploads when moving issue to another project

Closes #14531

See merge request !3382
parents 045112b7 cf21fd7a
...@@ -21,6 +21,9 @@ v 8.6.3 (unreleased) ...@@ -21,6 +21,9 @@ v 8.6.3 (unreleased)
- Destroy related todos when an Issue/MR is deleted. !3376 - Destroy related todos when an Issue/MR is deleted. !3376
- Fix error 500 when target is nil on todo list. !3376 - Fix error 500 when target is nil on todo list. !3376
v 8.6.3
- Fix copying uploads when moving issue to another project
v 8.6.2 v 8.6.2
- Fix dropdown alignment. !3298 - Fix dropdown alignment. !3298
- Fix issuable sidebar overlaps on tablet. !3299 - Fix issuable sidebar overlaps on tablet. !3299
......
...@@ -43,7 +43,7 @@ module Issues ...@@ -43,7 +43,7 @@ module Issues
def create_new_issue def create_new_issue
new_params = { id: nil, iid: nil, label_ids: [], milestone: nil, new_params = { id: nil, iid: nil, label_ids: [], milestone: nil,
project: @new_project, author: @old_issue.author, 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) new_params = @old_issue.serializable_hash.merge(new_params)
CreateService.new(@new_project, @current_user, new_params).execute CreateService.new(@new_project, @current_user, new_params).execute
...@@ -53,7 +53,7 @@ module Issues ...@@ -53,7 +53,7 @@ module Issues
@old_issue.notes.find_each do |note| @old_issue.notes.find_each do |note|
new_note = note.dup new_note = note.dup
new_params = { project: @new_project, noteable: @new_issue, new_params = { project: @new_project, noteable: @new_issue,
note: unfold_references(new_note.note), note: rewrite_content(new_note.note),
created_at: note.created_at, created_at: note.created_at,
updated_at: note.updated_at } updated_at: note.updated_at }
...@@ -61,6 +61,18 @@ module Issues ...@@ -61,6 +61,18 @@ module Issues
end end
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 def close_issue
close_service = CloseService.new(@old_project, @current_user) close_service = CloseService.new(@old_project, @current_user)
close_service.execute(@old_issue, notifications: false, system_note: false) close_service.execute(@old_issue, notifications: false, system_note: false)
...@@ -78,20 +90,12 @@ module Issues ...@@ -78,20 +90,12 @@ module Issues
direction: :to) direction: :to)
end end
def unfold_references(content) def mark_as_moved
return unless content @old_issue.update(moved_to: @new_issue)
rewriter = Gitlab::Gfm::ReferenceRewriter.new(content, @old_project,
@current_user)
rewriter.rewrite(@new_project)
end end
def notify_participants def notify_participants
notification_service.issue_moved(@old_issue, @new_issue, @current_user) notification_service.issue_moved(@old_issue, @new_issue, @current_user)
end end
def mark_as_moved
@old_issue.update(moved_to: @new_issue)
end
end end
end end
# encoding: utf-8 # encoding: utf-8
class FileUploader < CarrierWave::Uploader::Base class FileUploader < CarrierWave::Uploader::Base
include UploaderHelper include UploaderHelper
MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}
storage :file storage :file
attr_accessor :project, :secret attr_accessor :project, :secret
def initialize(project, secret = self.class.generate_secret) def initialize(project, secret = nil)
@project = project @project = project
@secret = secret @secret = secret || self.class.generate_secret
end end
def base_dir def base_dir
...@@ -23,14 +24,14 @@ class FileUploader < CarrierWave::Uploader::Base ...@@ -23,14 +24,14 @@ class FileUploader < CarrierWave::Uploader::Base
File.join(base_dir, 'tmp', @project.path_with_namespace, @secret) File.join(base_dir, 'tmp', @project.path_with_namespace, @secret)
end end
def self.generate_secret
SecureRandom.hex
end
def secure_url def secure_url
File.join("/uploads", @secret, file.filename) File.join("/uploads", @secret, file.filename)
end end
def to_markdown
to_h[:markdown]
end
def to_h def to_h
filename = image? ? self.file.basename : self.file.filename filename = image? ? self.file.basename : self.file.filename
escaped_filename = filename.gsub("]", "\\]") escaped_filename = filename.gsub("]", "\\]")
...@@ -45,4 +46,8 @@ class FileUploader < CarrierWave::Uploader::Base ...@@ -45,4 +46,8 @@ class FileUploader < CarrierWave::Uploader::Base
markdown: markdown markdown: markdown
} }
end end
def self.generate_secret
SecureRandom.hex
end
end end
...@@ -34,16 +34,21 @@ module Gitlab ...@@ -34,16 +34,21 @@ module Gitlab
@source_project = source_project @source_project = source_project
@current_user = current_user @current_user = current_user
@original_html = markdown(text) @original_html = markdown(text)
@pattern = Gitlab::ReferenceExtractor.references_pattern
end end
def rewrite(target_project) 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) unfold_reference(reference, Regexp.last_match, target_project)
end end
end end
def needs_rewrite?
@text =~ @pattern
end
private private
def unfold_reference(reference, match, target_project) def unfold_reference(reference, match, target_project)
......
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
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
require 'spec_helper' require 'spec_helper'
FactoryGirl.factories.map(&:name).each do |factory_name| describe 'factories' do
describe "#{factory_name} factory" do FactoryGirl.factories.each do |factory|
it 'should be valid' do describe "#{factory.name} factory" do
expect(build(factory_name)).to be_valid 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 end
end end
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
...@@ -160,6 +160,20 @@ describe Issues::MoveService, services: true do ...@@ -160,6 +160,20 @@ describe Issues::MoveService, services: true do
.to eq "Note with reference to merge request #{old_project.to_reference}!1" .to eq "Note with reference to merge request #{old_project.to_reference}!1"
end end
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 end
describe 'rewritting references' do describe 'rewritting references' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment