summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRobert Speicher <rspeicher@gmail.com>2017-02-15 13:11:44 -0500
committerRobert Speicher <rspeicher@gmail.com>2017-03-06 14:41:09 -0500
commit3a0be1c5fca6b80c75f728f7751b7c7614ab1bc0 (patch)
tree783932a4881ab39a4bd69adab15e7fa0376b2334
parent4c622b71fd284058deee483bf0009f8179b792bc (diff)
downloadgitlab-ce-3a0be1c5fca6b80c75f728f7751b7c7614ab1bc0.tar.gz
Add `RecordsUploads` module to record Upload records via callbacks
-rw-r--r--app/uploaders/attachment_uploader.rb1
-rw-r--r--app/uploaders/avatar_uploader.rb1
-rw-r--r--app/uploaders/file_uploader.rb6
-rw-r--r--app/uploaders/records_uploads.rb38
-rw-r--r--spec/uploaders/file_uploader_spec.rb2
-rw-r--r--spec/uploaders/records_uploads_spec.rb97
-rw-r--r--spec/uploaders/uploader_helper_spec.rb12
7 files changed, 151 insertions, 6 deletions
diff --git a/app/uploaders/attachment_uploader.rb b/app/uploaders/attachment_uploader.rb
index 6aa1f5a8c50..109eb2fea0b 100644
--- a/app/uploaders/attachment_uploader.rb
+++ b/app/uploaders/attachment_uploader.rb
@@ -1,4 +1,5 @@
class AttachmentUploader < GitlabUploader
+ include RecordsUploads
include UploaderHelper
storage :file
diff --git a/app/uploaders/avatar_uploader.rb b/app/uploaders/avatar_uploader.rb
index b4c393c6f2c..66d3bcb998a 100644
--- a/app/uploaders/avatar_uploader.rb
+++ b/app/uploaders/avatar_uploader.rb
@@ -1,4 +1,5 @@
class AvatarUploader < GitlabUploader
+ include RecordsUploads
include UploaderHelper
storage :file
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb
index 0d2edaeff3b..2cf97a1b6fd 100644
--- a/app/uploaders/file_uploader.rb
+++ b/app/uploaders/file_uploader.rb
@@ -1,5 +1,7 @@
class FileUploader < GitlabUploader
+ include RecordsUploads
include UploaderHelper
+
MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}
storage :file
@@ -20,6 +22,10 @@ class FileUploader < GitlabUploader
File.join(base_dir, 'tmp', @project.path_with_namespace, @secret)
end
+ def model
+ project
+ end
+
def to_markdown
to_h[:markdown]
end
diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb
new file mode 100644
index 00000000000..7a0424b5adf
--- /dev/null
+++ b/app/uploaders/records_uploads.rb
@@ -0,0 +1,38 @@
+module RecordsUploads
+ extend ActiveSupport::Concern
+
+ included do
+ after :store, :record_upload
+ before :remove, :destroy_upload
+ end
+
+ private
+
+ # After storing an attachment, create a corresponding Upload record
+ #
+ # NOTE: We're ignoring the argument passed to this callback because we want
+ # the `SanitizedFile` object from `CarrierWave::Uploader::Base#file`, not the
+ # `Tempfile` object the callback gets.
+ #
+ # Called `after :store`
+ def record_upload(_tempfile)
+ return unless file_storage?
+ return unless file.exists?
+
+ Upload.record(self)
+ end
+
+ # When removing an attachment, destroy any Upload records at the same path
+ #
+ # Note: this _will not work_ for Uploaders which relativize paths, such as
+ # `FileUploader`, but because that uploader uses different paths for every
+ # upload, that's an acceptable caveat.
+ #
+ # Called `before :remove`
+ def destroy_upload(*args)
+ return unless file_storage?
+ return unless file
+
+ Upload.remove_path(relative_path)
+ end
+end
diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb
index b0f5be55c33..396b0abdaef 100644
--- a/spec/uploaders/file_uploader_spec.rb
+++ b/spec/uploaders/file_uploader_spec.rb
@@ -1,7 +1,7 @@
require 'spec_helper'
describe FileUploader do
- let(:uploader) { described_class.new(build_stubbed(:project)) }
+ let(:uploader) { described_class.new(build_stubbed(:empty_project)) }
describe 'initialize' do
it 'generates a secret if none is provided' do
diff --git a/spec/uploaders/records_uploads_spec.rb b/spec/uploaders/records_uploads_spec.rb
new file mode 100644
index 00000000000..a104dc4257f
--- /dev/null
+++ b/spec/uploaders/records_uploads_spec.rb
@@ -0,0 +1,97 @@
+require 'rails_helper'
+
+describe RecordsUploads do
+ let(:uploader) do
+ example_uploader = Class.new(GitlabUploader) do
+ include RecordsUploads
+
+ storage :file
+
+ def model
+ FactoryGirl.build_stubbed(:user)
+ end
+ end
+
+ example_uploader.new
+ end
+
+ def upload_fixture(filename)
+ fixture_file_upload(Rails.root.join('spec', 'fixtures', filename))
+ end
+
+ describe 'callbacks' do
+ it 'calls `record_upload` after `store`' do
+ expect(uploader).to receive(:record_upload).once
+
+ uploader.store!(upload_fixture('doc_sample.txt'))
+ end
+
+ it 'calls `destroy_upload` after `remove`' do
+ expect(uploader).to receive(:destroy_upload).once
+
+ uploader.store!(upload_fixture('doc_sample.txt'))
+
+ uploader.remove!
+ end
+ end
+
+ describe '#record_upload callback' do
+ it 'returns early when not using file storage' do
+ allow(uploader).to receive(:file_storage?).and_return(false)
+ expect(Upload).not_to receive(:record)
+
+ uploader.store!(upload_fixture('rails_sample.jpg'))
+ end
+
+ it "returns early when the file doesn't exist" do
+ allow(uploader).to receive(:file).and_return(double(exists?: false))
+ expect(Upload).not_to receive(:record)
+
+ uploader.store!(upload_fixture('rails_sample.jpg'))
+ end
+
+ it 'creates an Upload record after store' do
+ expect(Upload).to receive(:record)
+ .with(uploader)
+
+ uploader.store!(upload_fixture('rails_sample.jpg'))
+ end
+
+ it 'it destroys Upload records at the same path before recording' do
+ existing = Upload.create!(
+ path: File.join(CarrierWave.root, 'uploads', 'rails_sample.jpg'),
+ size: 512.kilobytes,
+ model: build_stubbed(:user),
+ uploader: described_class.to_s
+ )
+
+ uploader.store!(upload_fixture('rails_sample.jpg'))
+
+ expect { existing.reload }.to raise_error(ActiveRecord::RecordNotFound)
+ expect(Upload.count).to eq 1
+ end
+ end
+
+ describe '#destroy_upload callback' do
+ it 'returns early when not using file storage' do
+ uploader.store!(upload_fixture('rails_sample.jpg'))
+
+ allow(uploader).to receive(:file_storage?).and_return(false)
+ expect(Upload).not_to receive(:remove_path)
+
+ uploader.remove!
+ end
+
+ it 'returns early when file is nil' do
+ expect(Upload).not_to receive(:remove_path)
+
+ uploader.remove!
+ end
+
+ it 'it destroys Upload records at the same path after removal' do
+ uploader.store!(upload_fixture('rails_sample.jpg'))
+
+ expect { uploader.remove! }.to change { Upload.count }.from(1).to(0)
+ end
+ end
+end
diff --git a/spec/uploaders/uploader_helper_spec.rb b/spec/uploaders/uploader_helper_spec.rb
index e9efd13b9aa..c47f09adb6d 100644
--- a/spec/uploaders/uploader_helper_spec.rb
+++ b/spec/uploaders/uploader_helper_spec.rb
@@ -1,10 +1,14 @@
require 'rails_helper'
describe UploaderHelper do
- class ExampleUploader < CarrierWave::Uploader::Base
- include UploaderHelper
+ let(:uploader) do
+ example_uploader = Class.new(CarrierWave::Uploader::Base) do
+ include UploaderHelper
- storage :file
+ storage :file
+ end
+
+ example_uploader.new
end
def upload_fixture(filename)
@@ -12,8 +16,6 @@ describe UploaderHelper do
end
describe '#image_or_video?' do
- let(:uploader) { ExampleUploader.new }
-
it 'returns true for an image file' do
uploader.store!(upload_fixture('dk.png'))