Commit 82326ad3 authored by Vijay Hawoldar's avatar Vijay Hawoldar

Fix bug with failed Snippet creation

Whereby the CreateService would return a deleted
snippet object to the controller, with an ID.
This would cause path generation issues in the resulting
form.
parent dc178732
...@@ -9,72 +9,77 @@ module Snippets ...@@ -9,72 +9,77 @@ module Snippets
def execute def execute
filter_spam_check_params filter_spam_check_params
snippet = if project @snippet = if project
project.snippets.build(params) project.snippets.build(params)
else else
PersonalSnippet.new(params) PersonalSnippet.new(params)
end end
unless Gitlab::VisibilityLevel.allowed_for?(current_user, snippet.visibility_level) unless Gitlab::VisibilityLevel.allowed_for?(current_user, @snippet.visibility_level)
deny_visibility_level(snippet) deny_visibility_level(@snippet)
return snippet_error_response(snippet, 403) return snippet_error_response(@snippet, 403)
end end
snippet.author = current_user @snippet.author = current_user
spam_check(snippet, current_user) spam_check(@snippet, current_user)
if save_and_commit(snippet) if save_and_commit
UserAgentDetailService.new(snippet, @request).create UserAgentDetailService.new(@snippet, @request).create
Gitlab::UsageDataCounters::SnippetCounter.count(:create) Gitlab::UsageDataCounters::SnippetCounter.count(:create)
ServiceResponse.success(payload: { snippet: snippet } ) ServiceResponse.success(payload: { snippet: @snippet } )
else else
snippet_error_response(snippet, 400) snippet_error_response(@snippet, 400)
end end
end end
private private
def save_and_commit(snippet) def save_and_commit
snippet_saved = snippet.with_transaction_returning_status do snippet_saved = @snippet.with_transaction_returning_status do
snippet.save && snippet.store_mentions! @snippet.save && @snippet.store_mentions!
end end
if snippet_saved && Feature.enabled?(:version_snippets, current_user) if snippet_saved && Feature.enabled?(:version_snippets, current_user)
create_repository_for(snippet) create_repository
create_commit(snippet) create_commit
end end
snippet_saved snippet_saved
rescue => e # Rescuing all because we can receive Creation exceptions, GRPC exceptions, Git exceptions, ... rescue => e # Rescuing all because we can receive Creation exceptions, GRPC exceptions, Git exceptions, ...
snippet.errors.add(:base, e.message)
log_error(e.message) log_error(e.message)
# If the commit action failed we need to remove the repository if exists # If the commit action failed we need to remove the repository if exists
snippet.repository.remove if snippet.repository_exists? @snippet.repository.remove if @snippet.repository_exists?
# If the snippet was created, we need to remove it as we # If the snippet was created, we need to remove it as we
# would do like if it had had any validation error # would do like if it had had any validation error
snippet.delete if snippet.persisted? # and reassign a dupe so we don't return the deleted snippet
if @snippet.persisted?
@snippet.delete
@snippet = @snippet.dup
end
@snippet.errors.add(:base, e.message)
false false
end end
def create_repository_for(snippet) def create_repository
snippet.create_repository @snippet.create_repository
raise CreateRepositoryError, 'Repository could not be created' unless snippet.repository_exists? raise CreateRepositoryError, 'Repository could not be created' unless @snippet.repository_exists?
end end
def create_commit(snippet) def create_commit
commit_attrs = { commit_attrs = {
branch_name: 'master', branch_name: 'master',
message: 'Initial commit' message: 'Initial commit'
} }
snippet.snippet_repository.multi_files_action(current_user, snippet_files, commit_attrs) @snippet.snippet_repository.multi_files_action(current_user, snippet_files, commit_attrs)
end end
def snippet_files def snippet_files
......
---
title: Resolve Snippet creation failure bug
merge_request: 27891
author:
type: fixed
...@@ -99,6 +99,11 @@ shared_examples_for 'snippet editor' do ...@@ -99,6 +99,11 @@ shared_examples_for 'snippet editor' do
it 'renders new page' do it 'renders new page' do
expect(page).to have_content('New Snippet') expect(page).to have_content('New Snippet')
end end
it 'has the correct action path' do
action = find('form.snippet-form')['action']
expect(action).to match(%r{/snippets\z})
end
end end
it 'validation fails for the first time' do it 'validation fails for the first time' do
......
...@@ -172,6 +172,10 @@ describe Snippets::CreateService do ...@@ -172,6 +172,10 @@ describe Snippets::CreateService do
it 'returns the error' do it 'returns the error' do
expect(snippet.errors.full_messages).to include('Repository could not be created') expect(snippet.errors.full_messages).to include('Repository could not be created')
end end
it 'does not return a snippet with an id' do
expect(snippet.id).to be_nil
end
end end
context 'when the commit action fails' do context 'when the commit action fails' do
......
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