Merge branch 'fj-208693-fix-bug-creating-snippet' into 'master'

Fix bug committing snippet content when creating the snippet

Closes #208693

See merge request gitlab-org/gitlab!26287

app/controllers/projects/snippets_controller.rb

@@ -52,8 +52,15 @@ class Projects::SnippetsController < Projects::ApplicationController
     create_params = snippet_params.merge(spammable_params)
     service_response = Snippets::CreateService.new(project, current_user, create_params).execute
     @snippet = service_response.payload[:snippet]
+    repository_operation_error = service_response.error? && !@snippet.persisted? && @snippet.valid?
 
-    recaptcha_check_with_fallback { render :new }
+    if repository_operation_error
+      flash.now[:alert] = service_response.message
+
+      render :new
+    else
+      recaptcha_check_with_fallback { render :new }
+    end
   end
 
   def update

app/controllers/snippets_controller.rb

@@ -52,10 +52,17 @@ class SnippetsController < ApplicationController
     create_params = snippet_params.merge(spammable_params)
     service_response = Snippets::CreateService.new(nil, current_user, create_params).execute
     @snippet = service_response.payload[:snippet]
+    repository_operation_error = service_response.error? && !@snippet.persisted? && @snippet.valid?
 
-    move_temporary_files if @snippet.valid? && params[:files]
+    if repository_operation_error
+      flash.now[:alert] = service_response.message
 
-    recaptcha_check_with_fallback { render :new }
+      render :new
+    else
+      move_temporary_files if @snippet.valid? && params[:files]
+
+      recaptcha_check_with_fallback { render :new }
+    end
   end
 
   def update

app/services/snippets/create_service.rb

@@ -38,25 +38,30 @@ module Snippets
     private
 
     def save_and_commit(snippet)
-      snippet.with_transaction_returning_status do
+      result = snippet.with_transaction_returning_status do
         (snippet.save && snippet.store_mentions!).tap do |saved|
           break false unless saved
 
           if Feature.enabled?(:version_snippets, current_user)
             create_repository_for(snippet)
-            create_commit(snippet)
           end
         end
-      rescue => e # Rescuing all because we can receive Creation exceptions, GRPC exceptions, Git exceptions, ...
-        snippet.errors.add(:base, e.message)
+      end
 
-        # If the commit action failed we need to remove the repository if exists
-        if snippet.repository_exists?
-          Repositories::DestroyService.new(snippet.repository).execute
-        end
+      create_commit(snippet) if result && snippet.repository_exists?
 
-        false
-      end
+      result
+    rescue => e # Rescuing all because we can receive Creation exceptions, GRPC exceptions, Git exceptions, ...
+      snippet.errors.add(:base, e.message)
+
+      # If the commit action failed we need to remove the repository if exists
+      snippet.repository.remove if snippet.repository_exists?
+
+      # If the snippet was created, we need to remove it as we
+      # would do like if it had had any validation error
+      snippet.delete if snippet.persisted?
+
+      false
     end
 
     def create_repository_for(snippet)

changelogs/unreleased/fj-208693-fix-bug-creating-snippet.yml

+---
+title: Fix bug committing snippet content when creating the snippet
+merge_request: 26287
+author:
+type: fixed

spec/features/projects/snippets/create_snippet_spec.rb

@@ -95,6 +95,29 @@ shared_examples_for 'snippet editor' do
       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})
     end
+
+    context 'when the git operation fails' do
+      let(:error) { 'This is a git error' }
+
+      before do
+        allow_next_instance_of(Snippets::CreateService) do |instance|
+          allow(instance).to receive(:create_commit).and_raise(StandardError, error)
+        end
+
+        fill_form
+
+        click_button('Create snippet')
+        wait_for_requests
+      end
+
+      it 'displays the error' do
+        expect(page).to have_content(error)
+      end
+
+      it 'renders new page' do
+        expect(page).to have_content('New Snippet')
+      end
+    end
   end
 
   context 'when a user is not authenticated' do

spec/features/snippets/user_creates_snippet_spec.rb

@@ -78,6 +78,29 @@ shared_examples_for 'snippet editor' do
     expect(reqs.first.status_code).to eq(200)
   end
 
+  context 'when the git operation fails' do
+    let(:error) { 'This is a git error' }
+
+    before do
+      allow_next_instance_of(Snippets::CreateService) do |instance|
+        allow(instance).to receive(:create_commit).and_raise(StandardError, error)
+      end
+
+      fill_form
+
+      click_button('Create snippet')
+      wait_for_requests
+    end
+
+    it 'displays the error' do
+      expect(page).to have_content(error)
+    end
+
+    it 'renders new page' do
+      expect(page).to have_content('New Snippet')
+    end
+  end
+
   it 'validation fails for the first time' do
     fill_in 'personal_snippet_title', with: 'My Snippet Title'
     click_button('Create snippet')

spec/services/snippets/create_service_spec.rb

@@ -185,12 +185,10 @@ describe Snippets::CreateService do
           expect { subject }.not_to change { Snippet.count }
         end
 
-        it 'does not create the repository' do
-          expect(snippet.repository_exists?).to be_falsey
-        end
-
-        it 'destroys the existing repository' do
-          expect(Repositories::DestroyService).to receive(:new).and_call_original
+        it 'destroys the created repository' do
+          expect_next_instance_of(Repository) do |instance|
+            expect(instance).to receive(:remove).and_call_original
+          end
 
           subject
         end