Merge branch 'fj-refactor-move-temporary-files' into 'master'

Refactor file upload in Personal Snippets

See merge request gitlab-org/gitlab!32291

app/controllers/snippets_controller.rb

@@ -49,23 +49,19 @@ class SnippetsController < ApplicationController
   end
 
   def create
-    create_params = snippet_params.merge(spammable_params)
+    create_params = snippet_params.merge(files: params.delete(:files))
     service_response = Snippets::CreateService.new(nil, current_user, create_params).execute
     @snippet = service_response.payload[:snippet]
 
     if service_response.error? && @snippet.errors[:repository].present?
       handle_repository_error(:new)
     else
-      move_temporary_files if @snippet.valid? && params[:files]
-
       recaptcha_check_with_fallback { render :new }
     end
   end
 
   def update
-    update_params = snippet_params.merge(spammable_params)
-
-    service_response = Snippets::UpdateService.new(nil, current_user, update_params).execute(@snippet)
+    service_response = Snippets::UpdateService.new(nil, current_user, snippet_params).execute(@snippet)
     @snippet = service_response.payload[:snippet]
 
     handle_repository_error(:edit)
@@ -150,12 +146,6 @@ class SnippetsController < ApplicationController
   end
 
   def snippet_params
-    params.require(:personal_snippet).permit(:title, :content, :file_name, :private, :visibility_level, :description)
-  end
-
-  def move_temporary_files
-    params[:files].each do |file|
-      FileMover.new(file, from_model: current_user, to_model: @snippet).execute
-    end
+    params.require(:personal_snippet).permit(:title, :content, :file_name, :private, :visibility_level, :description).merge(spammable_params)
   end
 end

app/services/snippets/create_service.rb

@@ -9,6 +9,8 @@ module Snippets
     def execute
       filter_spam_check_params
 
+      @files = Array(params.delete(:files).presence)
+
       @snippet = if project
                    project.snippets.build(params)
                  else
@@ -29,6 +31,8 @@ module Snippets
         UserAgentDetailService.new(@snippet, @request).create
         Gitlab::UsageDataCounters::SnippetCounter.count(:create)
 
+        move_temporary_files
+
         ServiceResponse.success(payload: { snippet: @snippet } )
       else
         snippet_error_response(@snippet, 400)
@@ -83,5 +87,13 @@ module Snippets
     def snippet_files
       [{ file_path: params[:file_name], content: params[:content] }]
     end
+
+    def move_temporary_files
+      return unless @snippet.is_a?(PersonalSnippet)
+
+      @files.each do |file|
+        FileMover.new(file, from_model: current_user, to_model: @snippet).execute
+      end
+    end
   end
 end

spec/controllers/snippets_controller_spec.rb

@@ -243,39 +243,13 @@ describe SnippetsController do
       end
     end
 
-    context 'when the snippet description contains a file' do
-      include FileMoverHelpers
+    context 'when the controller receives the files param' do
+      let(:files) { %w(foo bar) }
 
-      let(:picture_secret) { SecureRandom.hex }
-      let(:text_secret) { SecureRandom.hex }
-      let(:picture_file) { "/-/system/user/#{user.id}/#{picture_secret}/picture.jpg" }
-      let(:text_file) { "/-/system/user/#{user.id}/#{text_secret}/text.txt" }
-      let(:description) do
-        "Description with picture: ![picture](/uploads#{picture_file}) and "\
-        "text: [text.txt](/uploads#{text_file})"
-      end
-
-      before do
-        allow(FileUtils).to receive(:mkdir_p)
-        allow(FileUtils).to receive(:move)
-        stub_file_mover(text_file)
-        stub_file_mover(picture_file)
-      end
-
-      subject { create_snippet({ description: description }, { files: [picture_file, text_file] }) }
-
-      it 'creates the snippet' do
-        expect { subject }.to change { Snippet.count }.by(1)
-      end
-
-      it 'stores the snippet description correctly' do
-        snippet = subject
-
-        expected_description = "Description with picture: "\
-          "![picture](/uploads/-/system/personal_snippet/#{snippet.id}/#{picture_secret}/picture.jpg) and "\
-          "text: [text.txt](/uploads/-/system/personal_snippet/#{snippet.id}/#{text_secret}/text.txt)"
+      it 'passes the files param to the snippet create service' do
+        expect(Snippets::CreateService).to receive(:new).with(nil, user, hash_including(files: files)).and_call_original
 
-        expect(snippet.description).to eq(expected_description)
+        create_snippet({ title: nil }, { files: files })
       end
     end
 

spec/services/snippets/create_service_spec.rb

@@ -285,6 +285,20 @@ describe Snippets::CreateService do
       it_behaves_like 'an error service response when save fails'
       it_behaves_like 'creates repository and files'
       it_behaves_like 'after_save callback to store_mentions', ProjectSnippet
+
+      context 'when uploaded files are passed to the service' do
+        let(:extra_opts) { { files: ['foo'] } }
+
+        it 'does not move uploaded files to the snippet' do
+          expect_next_instance_of(described_class) do |instance|
+            expect(instance).to receive(:move_temporary_files).and_call_original
+          end
+
+          expect_any_instance_of(FileMover).not_to receive(:execute)
+
+          subject
+        end
+      end
     end
 
     context 'when PersonalSnippet' do
@@ -297,6 +311,51 @@ describe Snippets::CreateService do
       it_behaves_like 'an error service response when save fails'
       it_behaves_like 'creates repository and files'
       it_behaves_like 'after_save callback to store_mentions', PersonalSnippet
+
+      context 'when the snippet description contains files' do
+        include FileMoverHelpers
+
+        let(:title) { 'Title' }
+        let(:picture_secret) { SecureRandom.hex }
+        let(:text_secret) { SecureRandom.hex }
+        let(:picture_file) { "/-/system/user/#{creator.id}/#{picture_secret}/picture.jpg" }
+        let(:text_file) { "/-/system/user/#{creator.id}/#{text_secret}/text.txt" }
+        let(:files) { [picture_file, text_file] }
+        let(:description) do
+          "Description with picture: ![picture](/uploads#{picture_file}) and "\
+          "text: [text.txt](/uploads#{text_file})"
+        end
+
+        before do
+          allow(FileUtils).to receive(:mkdir_p)
+          allow(FileUtils).to receive(:move)
+        end
+
+        let(:extra_opts) { { description: description, title: title, files: files } }
+
+        it 'stores the snippet description correctly' do
+          stub_file_mover(text_file)
+          stub_file_mover(picture_file)
+
+          snippet = subject.payload[:snippet]
+
+          expected_description = "Description with picture: "\
+            "![picture](/uploads/-/system/personal_snippet/#{snippet.id}/#{picture_secret}/picture.jpg) and "\
+            "text: [text.txt](/uploads/-/system/personal_snippet/#{snippet.id}/#{text_secret}/text.txt)"
+
+          expect(snippet.description).to eq(expected_description)
+        end
+
+        context 'when there is a validation error' do
+          let(:title) { nil }
+
+          it 'does not move uploaded files to the snippet' do
+            expect_any_instance_of(described_class).not_to receive(:move_temporary_files)
+
+            subject
+          end
+        end
+      end
     end
   end
 end