diff --git a/app/assets/javascripts/dropzone_input.js b/app/assets/javascripts/dropzone_input.js index f886ce21493..8837341153b 100644 --- a/app/assets/javascripts/dropzone_input.js +++ b/app/assets/javascripts/dropzone_input.js @@ -199,7 +199,7 @@ window.DropzoneInput = (function() { }; addFileToForm = function(path) { - $(form).append(''); + $(form).append(''); }; getFilename = function(e) { diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index 1334f7daa44..6c25f59ccbb 100644 --- a/app/controllers/snippets_controller.rb +++ b/app/controllers/snippets_controller.rb @@ -45,7 +45,7 @@ class SnippetsController < ApplicationController @snippet = CreateSnippetService.new(nil, current_user, create_params).execute - move_temporary_files if params[:files] + move_temporary_files if @snippet.valid? && params[:files] recaptcha_check_with_fallback { render :new } end diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 5cb3de3d4f5..dc882b17143 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -17,6 +17,8 @@ class UploadsController < ApplicationController end def authorize_access! + return nil unless model + authorized = case model when Note @@ -35,7 +37,7 @@ class UploadsController < ApplicationController end def authorize_create_access! - return unless model + return nil unless model # for now we support only personal snippets comments authorized = can?(current_user, :comment_personal_snippet, model) @@ -77,7 +79,12 @@ class UploadsController < ApplicationController def uploader return @uploader if defined?(@uploader) - if model.is_a?(PersonalSnippet) + case model + when nil + @uploader = PersonalFileUploader.new(nil, params[:secret]) + + @uploader.retrieve_from_store!(params[:filename]) + when PersonalSnippet @uploader = PersonalFileUploader.new(model, params[:secret]) @uploader.retrieve_from_store!(params[:filename]) diff --git a/app/uploaders/file_mover.rb b/app/uploaders/file_mover.rb index 21e37a08a82..00c2888d224 100644 --- a/app/uploaders/file_mover.rb +++ b/app/uploaders/file_mover.rb @@ -1,33 +1,42 @@ class FileMover - attr_reader :secret, :file_name, :model + attr_reader :secret, :file_name, :model, :update_field def initialize(file_path, model, update_field = :description) @secret = File.split(File.dirname(file_path)).last @file_name = File.basename(file_path) @model = model + @update_field = update_field end def execute move - update_markdown + uploader.record_upload if update_markdown end private def move - FileUtils.mkdir_p(file_path) + FileUtils.mkdir_p(File.dirname(file_path)) FileUtils.move(temp_file_path, file_path) end - def update_markdown(field = :description) - updated_text = model.send(field).sub(temp_file_uploader.to_markdown, uploader.to_markdown) - model.update_attribute(field, updated_text) + def update_markdown + updated_text = model.read_attribute(update_field).gsub(temp_file_uploader.to_markdown, uploader.to_markdown) + model.update_attribute(update_field, updated_text) + + true + rescue + revert + + false end def temp_file_path + return @temp_file_path if @temp_file_path + temp_file_uploader.retrieve_from_store!(file_name) - temp_file_uploader.file.path + @temp_file_path = temp_file_uploader.file.path end def file_path @@ -45,4 +54,10 @@ class FileMover def temp_file_uploader @temp_file_uploader ||= PersonalFileUploader.new(nil, secret) end + + def revert + Rails.logger.warn("Markdown not updated, file move reverted for #{model}") + + FileUtils.move(file_path, temp_file_path) + end end diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb index 4c127f29250..feb4f04d7b7 100644 --- a/app/uploaders/records_uploads.rb +++ b/app/uploaders/records_uploads.rb @@ -6,8 +6,6 @@ module RecordsUploads 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 @@ -15,13 +13,16 @@ module RecordsUploads # `Tempfile` object the callback gets. # # Called `after :store` - def record_upload(_tempfile) + def record_upload(_tempfile = nil) + return unless model return unless file_storage? return unless file.exists? Upload.record(self) end + private + # Before removing an attachment, destroy any Upload records at the same path # # Called `before :remove` diff --git a/app/views/shared/form_elements/_description.html.haml b/app/views/shared/form_elements/_description.html.haml index 91224e232ca..307d4919224 100644 --- a/app/views/shared/form_elements/_description.html.haml +++ b/app/views/shared/form_elements/_description.html.haml @@ -2,7 +2,7 @@ - model = local_assigns.fetch(:model) - form = local_assigns.fetch(:form) -- supports_slash_commands = !model.persisted? +- supports_slash_commands = model.new_record? - if supports_slash_commands - preview_url = preview_markdown_path(project, slash_commands_target_type: model.class.name) diff --git a/app/views/shared/snippets/_header.html.haml b/app/views/shared/snippets/_header.html.haml index d2b94ed4c0b..813d8d69d8d 100644 --- a/app/views/shared/snippets/_header.html.haml +++ b/app/views/shared/snippets/_header.html.haml @@ -22,10 +22,9 @@ - if @snippet.updated_at != @snippet.created_at = edited_time_ago_with_tooltip(@snippet, placement: 'bottom', html_class: 'snippet-edited-ago', exclude_author: true) - %div - - if @snippet.description.present? - .description - .wiki - = markdown_field(@snippet, :description) - %textarea.hidden.js-task-list-field - = @snippet.description + - if @snippet.description.present? + .description + .wiki + = markdown_field(@snippet, :description) + %textarea.hidden.js-task-list-field + = @snippet.description diff --git a/config/routes/uploads.rb b/config/routes/uploads.rb index 76c31260394..ae8747d766d 100644 --- a/config/routes/uploads.rb +++ b/config/routes/uploads.rb @@ -9,6 +9,11 @@ scope path: :uploads do to: 'uploads#show', constraints: { model: /personal_snippet/, id: /\d+/, filename: /[^\/]+/ } + # show temporary uploads + get 'temp/:secret/:filename', + to: 'uploads#show', + constraints: { filename: /[^\/]+/ } + # Appearance get ":model/:mounted_as/:id/:filename", to: "uploads#show", diff --git a/doc/api/project_snippets.md b/doc/api/project_snippets.md index 80c4b5f1c34..92491de4daa 100644 --- a/doc/api/project_snippets.md +++ b/doc/api/project_snippets.md @@ -73,7 +73,7 @@ Parameters: - `file_name` (required) - The name of a snippet file - `description` (optional) - The description of a snippet - `code` (required) - The content of a snippet -- `visibility` (optional) - The snippet's visibility +- `visibility` (required) - The snippet's visibility ## Update snippet diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index 8000c9dec61..01a0659479b 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -92,6 +92,40 @@ describe UploadsController do end end end + + context 'temporal with valid image' do + subject do + post :create, model: 'personal_snippet', file: jpg, format: :json + end + + it 'returns a content with original filename, new link, and correct type.' do + subject + + expect(response.body).to match '\"alt\":\"rails_sample\"' + expect(response.body).to match "\"url\":\"/uploads/temp" + end + + it 'does not create an Upload record' do + expect { subject }.not_to change { Upload.count } + end + end + + context 'temporal with valid non-image file' do + subject do + post :create, model: 'personal_snippet', file: txt, format: :json + end + + it 'returns a content with original filename, new link, and correct type.' do + subject + + expect(response.body).to match '\"alt\":\"doc_sample.txt\"' + expect(response.body).to match "\"url\":\"/uploads/temp" + end + + it 'does not create an Upload record' do + expect { subject }.not_to change { Upload.count } + end + end end end diff --git a/spec/features/projects/snippets/create_snippet_spec.rb b/spec/features/projects/snippets/create_snippet_spec.rb index be9cc9f7029..5ac1ca45c74 100644 --- a/spec/features/projects/snippets/create_snippet_spec.rb +++ b/spec/features/projects/snippets/create_snippet_spec.rb @@ -27,7 +27,7 @@ feature 'Create Snippet', :js, feature: true do it 'creates a new snippet' do fill_form click_button('Create snippet') - wait_for_ajax + wait_for_requests expect(page).to have_content('My Snippet Title') expect(page).to have_content('Hello World!') @@ -44,7 +44,7 @@ feature 'Create Snippet', :js, feature: true do expect(page.find_field("project_snippet_description").value).to have_content('banana_sample') click_button('Create snippet') - wait_for_ajax + wait_for_requests link = find('a.no-attachment-icon img[alt="banana_sample"]')['src'] expect(link).to match(%r{/#{Regexp.escape(project.full_path) }/uploads/\h{32}/banana_sample\.gif\z}) @@ -60,7 +60,7 @@ feature 'Create Snippet', :js, feature: true do dropzone_file Rails.root.join('spec', 'fixtures', 'banana_sample.gif') click_button('Create snippet') - wait_for_ajax + wait_for_requests expect(page).to have_content('My Snippet Title') expect(page).to have_content('Hello World!') diff --git a/spec/features/snippets/create_snippet_spec.rb b/spec/features/snippets/create_snippet_spec.rb index f006057669b..ddd31ede064 100644 --- a/spec/features/snippets/create_snippet_spec.rb +++ b/spec/features/snippets/create_snippet_spec.rb @@ -30,6 +30,22 @@ feature 'Create Snippet', :js, feature: true do expect(page).to have_content('Hello World!') end + scenario 'previews a snippet with file' do + fill_in 'personal_snippet_description', with: 'My Snippet' + dropzone_file Rails.root.join('spec', 'fixtures', 'banana_sample.gif') + find('.js-md-preview-button').click + + page.within('#new_personal_snippet .md-preview') do + expect(page).to have_content('My Snippet') + + link = find('a.no-attachment-icon img[alt="banana_sample"]')['src'] + expect(link).to match(%r{/uploads/temp/\h{32}/banana_sample\.gif\z}) + + visit(link) + expect(page.status_code).to eq(200) + end + end + scenario 'uploads a file when dragging into textarea' do fill_form @@ -42,6 +58,9 @@ feature 'Create Snippet', :js, feature: true do link = find('a.no-attachment-icon img[alt="banana_sample"]')['src'] expect(link).to match(%r{/uploads/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z}) + + visit(link) + expect(page.status_code).to eq(200) end scenario 'validation fails for the first time' do @@ -64,6 +83,9 @@ feature 'Create Snippet', :js, feature: true do expect(page).to have_content('Hello World!') link = find('a.no-attachment-icon img[alt="banana_sample"]')['src'] expect(link).to match(%r{/uploads/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z}) + + visit(link) + expect(page.status_code).to eq(200) end scenario 'Authenticated user creates a snippet with + in filename' do diff --git a/spec/features/snippets/edit_snippet_spec.rb b/spec/features/snippets/edit_snippet_spec.rb index ee04792ef73..89ae593db88 100644 --- a/spec/features/snippets/edit_snippet_spec.rb +++ b/spec/features/snippets/edit_snippet_spec.rb @@ -13,14 +13,14 @@ feature 'Edit Snippet', :js, feature: true do login_as(user) visit edit_snippet_path(snippet) - wait_for_ajax + wait_for_requests end it 'updates the snippet' do fill_in 'personal_snippet_title', with: 'New Snippet Title' click_button('Save changes') - wait_for_ajax + wait_for_requests expect(page).to have_content('New Snippet Title') end @@ -30,7 +30,7 @@ feature 'Edit Snippet', :js, feature: true do expect(page.find_field("personal_snippet_description").value).to have_content('banana_sample') click_button('Save changes') - wait_for_ajax + wait_for_requests link = find('a.no-attachment-icon img[alt="banana_sample"]')['src'] expect(link).to match(%r{/uploads/personal_snippet/#{snippet.id}/\h{32}/banana_sample\.gif\z}) diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb index 99d50e78240..896cb410ed5 100644 --- a/spec/uploaders/file_mover_spec.rb +++ b/spec/uploaders/file_mover_spec.rb @@ -3,7 +3,10 @@ require 'spec_helper' describe FileMover do let(:filename) { 'banana_sample.gif' } let(:file) { fixture_file_upload(Rails.root.join('spec', 'fixtures', filename)) } - let(:temp_description) { 'test ![banana_sample](/uploads/temp/secret55/banana_sample.gif)' } + let(:temp_description) do + 'test ![banana_sample](/uploads/temp/secret55/banana_sample.gif) same ![banana_sample]'\ + '(/uploads/temp/secret55/banana_sample.gif)' + end let(:temp_file_path) { File.join('secret55', filename).to_s } let(:file_path) { File.join('uploads', 'personal_snippet', snippet.id.to_s, 'secret55', filename).to_s } @@ -12,14 +15,49 @@ describe FileMover do subject { described_class.new(file_path, snippet).execute } describe '#execute' do - it 'updates the description correctly' do - expect(FileUtils).to receive(:mkdir_p).with(a_string_including(file_path)) + before do + expect(FileUtils).to receive(:mkdir_p).with(a_string_including(File.dirname(file_path))) expect(FileUtils).to receive(:move).with(a_string_including(temp_file_path), a_string_including(file_path)) + allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?).and_return(true) + allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:size).and_return(10) + end - subject + context 'when move and field update successful' do + it 'updates the description correctly' do + subject - expect(snippet.reload.description) - .to eq("test ![banana_sample](/uploads/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)") + expect(snippet.reload.description) + .to eq( + "test ![banana_sample](/uploads/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)"\ + " same ![banana_sample](/uploads/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)" + ) + end + + it 'creates a new update record' do + expect { subject }.to change { Upload.count }.by(1) + end + end + + context 'when update_markdown fails' do + before do + expect(FileUtils).to receive(:move).with(a_string_including(file_path), a_string_including(temp_file_path)) + end + + subject { described_class.new(file_path, snippet, :non_existing_field).execute } + + it 'does not update the description' do + subject + + expect(snippet.reload.description) + .to eq( + "test ![banana_sample](/uploads/temp/secret55/banana_sample.gif)"\ + " same ![banana_sample](/uploads/temp/secret55/banana_sample.gif)" + ) + end + + it 'does not create a new update record' do + expect { subject }.not_to change { Upload.count } + end end end end diff --git a/spec/uploaders/records_uploads_spec.rb b/spec/uploaders/records_uploads_spec.rb index 5c26e334a6e..bb32ee62ccb 100644 --- a/spec/uploaders/records_uploads_spec.rb +++ b/spec/uploaders/records_uploads_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' describe RecordsUploads do - let(:uploader) do + let!(:uploader) do class RecordsUploadsExampleUploader < GitlabUploader include RecordsUploads @@ -57,6 +57,13 @@ describe RecordsUploads do uploader.store!(upload_fixture('rails_sample.jpg')) end + it 'does not create an Upload record if model is missing' do + expect_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil) + expect(Upload).not_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('uploads', 'rails_sample.jpg'),