1 files changed, 22 insertions, 25 deletions

diff --git a/app/services/snippets/update_service.rb b/app/services/snippets/update_service.rb
index e56b20c6057..2dc9266dbd0 100644
--- a/app/services/snippets/update_service.rb
+++ b/app/services/snippets/update_service.rb
@@ -2,24 +2,15 @@
 
 module Snippets
   class UpdateService < Snippets::BaseService
-    include SpamCheckMethods
+    COMMITTABLE_ATTRIBUTES = %w(file_name content).freeze
 
     UpdateError = Class.new(StandardError)
-    CreateRepositoryError = Class.new(StandardError)
 
     def execute(snippet)
-      # check that user is allowed to set specified visibility_level
-      new_visibility = visibility_level
-
-      if new_visibility && new_visibility.to_i != snippet.visibility_level
-        unless Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
-          deny_visibility_level(snippet, new_visibility)
-
-          return snippet_error_response(snippet, 403)
-        end
+      if visibility_changed?(snippet) && !visibility_allowed?(snippet, visibility_level)
+        return error_forbidden_visibility(snippet)
       end
 
-      filter_spam_check_params
       snippet.assign_attributes(params)
       spam_check(snippet, current_user)
 
@@ -34,30 +25,32 @@ module Snippets
 
     private
 
+    def visibility_changed?(snippet)
+      visibility_level && visibility_level.to_i != snippet.visibility_level
+    end
+
     def save_and_commit(snippet)
       return false unless snippet.save
 
-      # In order to avoid non migrated snippets scenarios,
-      # if the snippet does not have a repository we created it
-      # We don't need to check if the repository exists
-      # because `create_repository` already handles it
-      if Feature.enabled?(:version_snippets, current_user)
-        create_repository_for(snippet)
-      end
+      # If the updated attributes does not need to update
+      # the repository we can just return
+      return true unless committable_attributes?
 
-      # If the snippet repository exists we commit always
-      # the changes
-      create_commit(snippet) if snippet.repository_exists?
+      create_repository_for(snippet)
+      create_commit(snippet)
 
       true
     rescue => e
-      # Restore old attributes
+      # Restore old attributes but re-assign changes so they're not lost
       unless snippet.previous_changes.empty?
         snippet.previous_changes.each { |attr, value| snippet[attr] = value[0] }
         snippet.save
+
+        snippet.assign_attributes(params)
       end
 
-      snippet.errors.add(:repository, 'Error updating the snippet')
+      add_snippet_repository_error(snippet: snippet, error: e)
+
       log_error(e.message)
 
       # If the commit action failed we remove it because
@@ -92,7 +85,7 @@ module Snippets
     end
 
     def snippet_files(snippet)
-      [{ previous_path: snippet.blobs.first&.path,
+      [{ previous_path: snippet.file_name_on_repo,
          file_path: params[:file_name],
          content: params[:content] }]
     end
@@ -104,5 +97,9 @@ module Snippets
     def repository_empty?(snippet)
       snippet.repository._uncached_exists? && !snippet.repository._uncached_has_visible_content?
     end
+
+    def committable_attributes?
+      (params.stringify_keys.keys & COMMITTABLE_ATTRIBUTES).present?
+    end
   end
 end