Merge branch 'fj-add-snippet-files-param-to-snippet-update-service' into 'master'

Refactor Snippets::UpdateService to allow multiple files

See merge request gitlab-org/gitlab!32832

app/models/snippet_input_action.rb

@@ -28,9 +28,17 @@ class SnippetInputAction
   def to_commit_action
     {
       action: action&.to_sym,
-      previous_path: previous_path,
+      previous_path: build_previous_path,
       file_path: file_path,
       content: content
     }
   end
+
+  private
+
+  def build_previous_path
+    return previous_path unless update_action?
+
+    previous_path.presence || file_path
+  end
 end

app/models/snippet_input_action_collection.rb

@@ -5,7 +5,7 @@ class SnippetInputActionCollection
 
   attr_reader :actions
 
-  delegate :empty?, to: :actions
+  delegate :empty?, :any?, :[], to: :actions
 
   def initialize(actions = [])
     @actions = actions.map { |action| SnippetInputAction.new(action) }

app/services/snippets/base_service.rb

@@ -72,11 +72,11 @@ module Snippets
       message
     end
 
-    def files_to_commit
-      snippet_files.to_commit_actions.presence || build_actions_from_params
+    def files_to_commit(snippet)
+      snippet_files.to_commit_actions.presence || build_actions_from_params(snippet)
     end
 
-    def build_actions_from_params
+    def build_actions_from_params(snippet)
       raise NotImplementedError
     end
   end

app/services/snippets/create_service.rb

@@ -43,9 +43,7 @@ module Snippets
     def create_params
       return params if snippet_files.empty?
 
-      first_file = snippet_files.actions.first
-
-      params.merge(content: first_file.content, file_name: first_file.file_path)
+      params.merge(content: snippet_files[0].content, file_name: snippet_files[0].file_path)
     end
 
     def save_and_commit
@@ -88,7 +86,7 @@ module Snippets
         message: 'Initial commit'
       }
 
-      @snippet.snippet_repository.multi_files_action(current_user, files_to_commit, commit_attrs)
+      @snippet.snippet_repository.multi_files_action(current_user, files_to_commit(@snippet), commit_attrs)
     end
 
     def move_temporary_files
@@ -99,7 +97,7 @@ module Snippets
       end
     end
 
-    def build_actions_from_params
+    def build_actions_from_params(_snippet)
       [{ file_path: params[:file_name], content: params[:content] }]
     end
   end

app/services/snippets/update_service.rb

@@ -7,11 +7,13 @@ module Snippets
     UpdateError = Class.new(StandardError)
 
     def execute(snippet)
+      return invalid_params_error(snippet) unless valid_params?
+
       if visibility_changed?(snippet) && !visibility_allowed?(snippet, visibility_level)
         return forbidden_visibility_error(snippet)
       end
 
-      snippet.assign_attributes(params)
+      update_snippet_attributes(snippet)
       spam_check(snippet, current_user)
 
       if save_and_commit(snippet)
@@ -29,6 +31,19 @@ module Snippets
       visibility_level && visibility_level.to_i != snippet.visibility_level
     end
 
+    def update_snippet_attributes(snippet)
+      # We can remove the following condition once
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/217801
+      # is implemented.
+      # Once we can perform different operations through this service
+      # we won't need to keep track of the `content` and `file_name` fields
+      if snippet_files.any?
+        params.merge!(content: snippet_files[0].content, file_name: snippet_files[0].file_path)
+      end
+
+      snippet.assign_attributes(params)
+    end
+
     def save_and_commit(snippet)
       return false unless snippet.save
 
@@ -81,13 +96,7 @@ module Snippets
         message: 'Update snippet'
       }
 
-      snippet.snippet_repository.multi_files_action(current_user, snippet_files(snippet), commit_attrs)
-    end
-
-    def snippet_files(snippet)
-      [{ previous_path: snippet.file_name_on_repo,
-         file_path: params[:file_name],
-         content: params[:content] }]
+      snippet.snippet_repository.multi_files_action(current_user, files_to_commit(snippet), commit_attrs)
     end
 
     # Because we are removing repositories we don't want to remove
@@ -99,7 +108,13 @@ module Snippets
     end
 
     def committable_attributes?
-      (params.stringify_keys.keys & COMMITTABLE_ATTRIBUTES).present?
+      (params.stringify_keys.keys & COMMITTABLE_ATTRIBUTES).present? || snippet_files.any?
+    end
+
+    def build_actions_from_params(snippet)
+      [{ previous_path: snippet.file_name_on_repo,
+         file_path: params[:file_name],
+         content: params[:content] }]
     end
   end
 end

changelogs/unreleased/fj-add-snippet-files-param-to-snippet-update-service.yml

+---
+title: Allow the snippet update service to accept an array of files
+merge_request: 32832
+author:
+type: changed

spec/models/snippet_input_action_collection_spec.rb

@@ -8,6 +8,8 @@ describe SnippetInputActionCollection do
   let(:data)           { [action, action] }
 
   it { is_expected.to delegate_method(:empty?).to(:actions) }
+  it { is_expected.to delegate_method(:any?).to(:actions) }
+  it { is_expected.to delegate_method(:[]).to(:actions) }
 
   describe '#to_commit_actions' do
     subject { described_class.new(data).to_commit_actions}

spec/models/snippet_input_action_spec.rb

@@ -35,11 +35,31 @@ describe SnippetInputAction do
     let(:content)          { 'bar' }
     let(:previous_path)    { 'previous_path' }
     let(:options)          { { action: action, file_path: file_path, content: content, previous_path: previous_path } }
+    let(:expected_options) { options.merge(action: action.to_sym) }
 
     subject { described_class.new(options).to_commit_action }
 
     it 'transforms attributes to commit action' do
-      expect(subject).to eq(options.merge(action: action.to_sym))
+      expect(subject).to eq(expected_options)
+    end
+
+    context 'action is update' do
+      let(:action) { 'update' }
+
+      context 'when previous_path is present' do
+        it 'returns the existing previous_path' do
+          expect(subject).to eq(expected_options)
+        end
+      end
+
+      context 'when previous_path is not present' do
+        let(:previous_path) { nil }
+        let(:expected_options) { options.merge(action: action.to_sym, previous_path: file_path) }
+
+        it 'assigns the file_path to the previous_path' do
+          expect(subject).to eq(expected_options)
+        end
+      end
     end
   end
 end

spec/services/snippets/update_service_spec.rb

@@ -302,6 +302,82 @@ describe Snippets::UpdateService do
       end
     end
 
+    shared_examples 'when snippet_files param is present' do
+      let(:file_path) { 'CHANGELOG' }
+      let(:content) { 'snippet_content' }
+      let(:new_title) { 'New title' }
+      let(:snippet_files) { [{ action: 'update', previous_path: file_path, file_path: file_path, content: content }] }
+      let(:base_opts) do
+        {
+          title: new_title,
+          snippet_files: snippet_files
+        }
+      end
+
+      it 'updates a snippet with the provided attributes' do
+        file_path = 'foo'
+        snippet_files[0][:action] = 'move'
+        snippet_files[0][:file_path] = file_path
+
+        response = subject
+        snippet = response.payload[:snippet]
+
+        expect(response).to be_success
+        expect(snippet.title).to eq(new_title)
+        expect(snippet.file_name).to eq(file_path)
+        expect(snippet.content).to eq(content)
+      end
+
+      it 'commit the files to the repository' do
+        subject
+
+        blob = snippet.repository.blob_at('master', file_path)
+
+        expect(blob.data).to eq content
+      end
+
+      context 'when content or file_name params are present' do
+        let(:extra_opts) { { content: 'foo', file_name: 'path' } }
+
+        it 'raises a validation error' do
+          response = subject
+          snippet = response.payload[:snippet]
+
+          expect(response).to be_error
+          expect(snippet.errors.full_messages_for(:content)).to eq ['Content and snippet files cannot be used together']
+          expect(snippet.errors.full_messages_for(:file_name)).to eq ['File name and snippet files cannot be used together']
+        end
+      end
+
+      context 'when snippet_files param is invalid' do
+        let(:snippet_files) { [{ action: 'invalid_action' }] }
+
+        it 'raises a validation error' do
+          response = subject
+          snippet = response.payload[:snippet]
+
+          expect(response).to be_error
+          expect(snippet.errors.full_messages_for(:snippet_files)).to eq ['Snippet files have invalid data']
+        end
+      end
+
+      context 'when an error is raised committing the file' do
+        it 'keeps any snippet modifications' do
+          expect_next_instance_of(described_class) do |instance|
+            expect(instance).to receive(:create_repository_for).and_raise(StandardError)
+          end
+
+          response = subject
+          snippet = response.payload[:snippet]
+
+          expect(response).to be_error
+          expect(snippet.title).to eq(new_title)
+          expect(snippet.file_name).to eq(file_path)
+          expect(snippet.content).to eq(content)
+        end
+      end
+    end
+
     context 'when Project Snippet' do
       let_it_be(:project) { create(:project) }
       let!(:snippet) { create(:project_snippet, :repository, author: user, project: project) }
@@ -316,6 +392,7 @@ describe Snippets::UpdateService do
       it_behaves_like 'updates repository content'
       it_behaves_like 'commit operation fails'
       it_behaves_like 'committable attributes'
+      it_behaves_like 'when snippet_files param is present'
       it_behaves_like 'snippets spam check is performed' do
         before do
           subject
@@ -340,6 +417,7 @@ describe Snippets::UpdateService do
       it_behaves_like 'updates repository content'
       it_behaves_like 'commit operation fails'
       it_behaves_like 'committable attributes'
+      it_behaves_like 'when snippet_files param is present'
       it_behaves_like 'snippets spam check is performed' do
         before do
           subject