summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2016-04-01 07:17:10 +0000
committerRémy Coutable <remy@rymai.me>2016-04-01 10:01:22 +0200
commit36c8506b9b674fcfab156a63042a942e33de3670 (patch)
treed91ef947e8a0d0d65acc5e6e530789ade6145b1e
parent2a5bfc461e0c828cf61aeb8b7ca903fa3ce8fbee (diff)
downloadgitlab-ce-36c8506b9b674fcfab156a63042a942e33de3670.tar.gz
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>
-rw-r--r--CHANGELOG1
-rw-r--r--app/services/issues/move_service.rb31
-rw-r--r--app/uploaders/file_uploader.rb17
-rw-r--r--lib/gitlab/gfm/reference_rewriter.rb9
-rw-r--r--lib/gitlab/gfm/uploads_rewriter.rb51
-rw-r--r--spec/factories/file_uploader.rb20
-rw-r--r--spec/factories_spec.rb16
-rw-r--r--spec/lib/gitlab/gfm/uploads_rewriter_spec.rb66
-rw-r--r--spec/services/issues/move_service_spec.rb14
9 files changed, 200 insertions, 25 deletions
diff --git a/CHANGELOG b/CHANGELOG
index 5aca2ed20e9..44e789bd800 100644
--- a/CHANGELOG
+++ b/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
diff --git a/app/services/issues/move_service.rb b/app/services/issues/move_service.rb
index 468f8acdf64..82e7090f1ea 100644
--- a/app/services/issues/move_service.rb
+++ b/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
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb
index 86d24469e05..1af9e9b0edb 100644
--- a/app/uploaders/file_uploader.rb
+++ b/app/uploaders/file_uploader.rb
@@ -1,14 +1,15 @@
# 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
diff --git a/lib/gitlab/gfm/reference_rewriter.rb b/lib/gitlab/gfm/reference_rewriter.rb
index a1c6ee7bd69..78d7a4f27cf 100644
--- a/lib/gitlab/gfm/reference_rewriter.rb
+++ b/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)
diff --git a/lib/gitlab/gfm/uploads_rewriter.rb b/lib/gitlab/gfm/uploads_rewriter.rb
new file mode 100644
index 00000000000..abc8c8c55e6
--- /dev/null
+++ b/lib/gitlab/gfm/uploads_rewriter.rb
@@ -0,0 +1,51 @@
+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
diff --git a/spec/factories/file_uploader.rb b/spec/factories/file_uploader.rb
new file mode 100644
index 00000000000..1b36e21f2b0
--- /dev/null
+++ b/spec/factories/file_uploader.rb
@@ -0,0 +1,20 @@
+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
diff --git a/spec/factories_spec.rb b/spec/factories_spec.rb
index 457859dedaf..62de081661d 100644
--- a/spec/factories_spec.rb
+++ b/spec/factories_spec.rb
@@ -1,9 +1,17 @@
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
diff --git a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb
new file mode 100644
index 00000000000..eda956e6f0a
--- /dev/null
+++ b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb
@@ -0,0 +1,66 @@
+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
diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb
index ade3b7850f1..28337f249a7 100644
--- a/spec/services/issues/move_service_spec.rb
+++ b/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