Refactor Snippets::UpdateService to allow multiple files

In this commit we adapt the `Snippets::UpdateService` to
accept an array param that will store the snippet files.

This param will be used by GraphQL and the REST endpoints.
parent dab25026
......@@ -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
......@@ -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) }
......
......@@ -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
......
......@@ -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
......
......@@ -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
---
title: Allow the snippet update service to accept an array of files
merge_request: 32832
author:
type: changed
......@@ -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}
......
......@@ -30,16 +30,36 @@ describe SnippetInputAction do
end
describe '#to_commit_action' do
let(:action) { 'create' }
let(:file_path) { 'foo' }
let(:content) { 'bar' }
let(:previous_path) { 'previous_path' }
let(:options) { { action: action, file_path: file_path, content: content, previous_path: previous_path } }
let(:action) { 'create' }
let(:file_path) { 'foo' }
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
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment