Updates from last code review:38356-add-last_commit_sha-to-the-commit-api

- Apply some refactoring for code reuse
- Add file status validation for Files::DeleteService
- Write additional specs

4 files changed, 33 insertions, 27 deletions

diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb
index 38231f66009..8d4b9f14780 100644
--- a/app/services/files/base_service.rb
+++ b/app/services/files/base_service.rb
@@ -1,11 +1,14 @@
 module Files
   class BaseService < Commits::CreateService
+    FileChangedError = Class.new(StandardError)
+
     def initialize(*args)
       super
 
       @author_email = params[:author_email]
       @author_name = params[:author_name]
       @commit_message = params[:commit_message]
+      @last_commit_sha = params[:last_commit_sha]
 
       @file_path = params[:file_path]
       @previous_path = params[:previous_path]
@@ -13,5 +16,16 @@ module Files
       @file_content = params[:file_content]
       @file_content = Base64.decode64(@file_content) if params[:file_content_encoding] == 'base64'
     end
+
+    def file_has_changed?(path, commit_id)
+      return false unless commit_id
+
+      last_commit = Gitlab::Git::Commit
+        .last_for_path(@start_project.repository, @start_branch, path)
+
+      return false unless last_commit
+
+      last_commit.sha != commit_id
+    end
   end
 end
diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb
index 7952e5c95d4..32a57484d4e 100644
--- a/app/services/files/delete_service.rb
+++ b/app/services/files/delete_service.rb
@@ -11,5 +11,15 @@ module Files
         start_project: @start_project,
         start_branch_name: @start_branch)
     end
+
+    private
+
+    def validate!
+      super
+
+      if file_has_changed?(@file_path, @last_commit_sha)
+        raise FileChangedError, "You are attempting to delete a file that has been previously updated."
+      end
+    end
   end
 end
diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb
index eb2773733b1..98a3e83c130 100644
--- a/app/services/files/multi_service.rb
+++ b/app/services/files/multi_service.rb
@@ -1,5 +1,7 @@
 module Files
   class MultiService < Files::BaseService
+    UPDATE_FILE_ACTIONS = %w(update move delete).freeze
+
     def create_commit!
       repository.multi_action(
         user: current_user,
@@ -20,7 +22,7 @@ module Files
 
       params[:actions].each do |action|
         validate_action!(action)
-        validate_file_status(action)
+        validate_file_status!(action)
       end
     end
 
@@ -30,14 +32,13 @@ module Files
       end
     end
 
-    def validate_file_status(action)
-      return unless action[:last_commit_id]
+    def validate_file_status!(action)
+      return unless UPDATE_FILE_ACTIONS.include?(action[:action])
 
-      current_commit = Gitlab::Git::Commit.last_for_path(
-        @start_project.repository, @start_branch, action[:file_path])
+      file_path = action[:previous_path] || action[:file_path]
 
-      if current_commit.sha != action[:last_commit_id]
-        raise_error("The file has changed since you started editing it: #{action[:file_path]}")
+      if file_has_changed?(file_path, action[:last_commit_id])
+        raise_error("The file has changed since you started editing it: #{file_path}")
       end
     end
   end
diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb
index bcca1386bed..1902d1cea72 100644
--- a/app/services/files/update_service.rb
+++ b/app/services/files/update_service.rb
@@ -1,13 +1,5 @@
 module Files
   class UpdateService < Files::BaseService
-    FileChangedError = Class.new(StandardError)
-
-    def initialize(*args)
-      super
-
-      @last_commit_sha = params[:last_commit_sha]
-    end
-
     def create_commit!
       repository.update_file(current_user, @file_path, @file_content,
                              message: @commit_message,
@@ -21,21 +13,10 @@ module Files
 
     private
 
-    def file_has_changed?
-      return false unless @last_commit_sha && last_commit
-
-      @last_commit_sha != last_commit.sha
-    end
-
-    def last_commit
-      @last_commit ||= Gitlab::Git::Commit
-        .last_for_path(@start_project.repository, @start_branch, @file_path)
-    end
-
     def validate!
       super
 
-      if file_has_changed?
+      if file_has_changed?(@file_path, @last_commit_sha)
         raise FileChangedError, "You are attempting to update a file that has changed since you started editing it."
       end
     end