Support uploads for newly created personal snippets12910-snippets-description

15 files changed, 164 insertions, 36 deletions

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('<input type="hidden" name="files[]" value="' + path + '">');
+      $(form).append('<input type="hidden" name="files[]" value="' + _.escape(path) + '">');
     };
 
     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
+
+    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)"\
+            " 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
 
-      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
 
-      expect(snippet.reload.description)
-        .to eq("test ![banana_sample](/uploads/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)")
+      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'),