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

8 files changed, 153 insertions, 33 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
diff --git a/doc/api/commits.md b/doc/api/commits.md
index 7de08d230dd..c9b72d4a1dd 100644
--- a/doc/api/commits.md
+++ b/doc/api/commits.md
@@ -84,7 +84,7 @@ POST /projects/:id/repository/commits
 | `previous_path` | string | no | Original full path to the file being moved. Ex. `lib/class1.rb` |
 | `content` | string | no | File content, required for all except `delete`. Optional for `move` |
 | `encoding` | string | no | `text` or `base64`. `text` is default. |
-| `last_commit_id` | string | no | Last known file commit id |
+| `last_commit_id` | string | no | Last known file commit id. Will be only considered in update, move and delete actions. |
 
 ```bash
 PAYLOAD=$(cat << 'JSON'
diff --git a/doc/api/repository_files.md b/doc/api/repository_files.md
index c517a38a8ba..a1a0b1b756c 100644
--- a/doc/api/repository_files.md
+++ b/doc/api/repository_files.md
@@ -151,3 +151,4 @@ Parameters:
 - `author_email` (optional) - Specify the commit author's email address
 - `author_name` (optional) - Specify the commit author's name
 - `commit_message` (required) - Commit message
+- `last_commit_id` (optional) - Last known file commit id
diff --git a/spec/services/files/delete_service_spec.rb b/spec/services/files/delete_service_spec.rb
new file mode 100644
index 00000000000..e9f8f0efe6b
--- /dev/null
+++ b/spec/services/files/delete_service_spec.rb
@@ -0,0 +1,64 @@
+require "spec_helper"
+
+describe Files::DeleteService do
+  subject { described_class.new(project, user, commit_params) }
+
+  let(:project) { create(:project, :repository) }
+  let(:user) { create(:user) }
+  let(:file_path) { 'files/ruby/popen.rb' }
+  let(:branch_name) { project.default_branch }
+  let(:last_commit_sha) { nil }
+
+  let(:commit_params) do
+    {
+      file_path: file_path,
+      commit_message: "Delete File",
+      last_commit_sha: last_commit_sha,
+      start_project: project,
+      start_branch: project.default_branch,
+      branch_name: branch_name
+    }
+  end
+
+  shared_examples 'successfully deletes the file' do
+    it 'returns a hash with the :success status' do
+      results = subject.execute
+
+      expect(results[:status]).to match(:success)
+    end
+
+    it 'deletes the file' do
+      subject.execute
+
+      blob = project.repository.blob_at_branch(project.default_branch, file_path)
+
+      expect(blob).to be_nil
+    end
+  end
+
+  before do
+    project.team << [user, :master]
+  end
+
+  describe "#execute" do
+    context "when the file's last commit sha does not match the supplied last_commit_sha" do
+      let(:last_commit_sha) { "foo" }
+
+      it "returns a hash with the correct error message and a :error status " do
+        expect { subject.execute }
+          .to raise_error(Files::UpdateService::FileChangedError,
+                         "You are attempting to delete a file that has been previously updated.")
+      end
+    end
+
+    context "when the file's last commit sha does match the supplied last_commit_sha" do
+      let(:last_commit_sha) { Gitlab::Git::Commit.last_for_path(project.repository, project.default_branch, file_path).sha }
+
+      it_behaves_like 'successfully deletes the file'
+    end
+
+    context "when the last_commit_sha is not supplied" do
+      it_behaves_like 'successfully deletes the file'
+    end
+  end
+end
diff --git a/spec/services/files/multi_service_spec.rb b/spec/services/files/multi_service_spec.rb
index 6e11f126f14..085a28d267f 100644
--- a/spec/services/files/multi_service_spec.rb
+++ b/spec/services/files/multi_service_spec.rb
@@ -6,18 +6,20 @@ describe Files::MultiService do
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
   let(:branch_name) { project.default_branch }
-  let(:file_path) { 'files/ruby/popen.rb' }
+  let(:original_file_path) { 'files/ruby/popen.rb' }
+  let(:new_file_path) { 'files/ruby/popen.rb' }
   let(:action) { 'update' }
 
   let!(:original_commit_id) do
-    Gitlab::Git::Commit.last_for_path(project.repository, branch_name, file_path).sha
+    Gitlab::Git::Commit.last_for_path(project.repository, branch_name, original_file_path).sha
   end
 
   let(:actions) do
     [
       {
         action: action,
-        file_path: file_path,
+        file_path: new_file_path,
+        previous_path: original_file_path,
         content: 'New content',
         last_commit_id: original_commit_id
       }
@@ -60,14 +62,14 @@ describe Files::MultiService do
     describe 'Updating files' do
       context 'when the file has been previously updated' do
         before do
-          update_file(file_path)
+          update_file(original_file_path)
         end
 
         it 'rejects the commit' do
           results = subject.execute
 
           expect(results[:status]).to eq(:error)
-          expect(results[:message]).to match(file_path)
+          expect(results[:message]).to match(new_file_path)
         end
       end
 
@@ -79,6 +81,53 @@ describe Files::MultiService do
         end
       end
     end
+
+    context 'when moving a file' do
+      let(:action) { 'move' }
+      let(:new_file_path) { 'files/ruby/new_popen.rb' }
+
+      context 'when original file has been updated' do
+        before do
+          update_file(original_file_path)
+        end
+
+        it 'rejects the commit' do
+          results = subject.execute
+
+          expect(results[:status]).to eq(:error)
+          expect(results[:message]).to match(original_file_path)
+        end
+      end
+
+      context 'when original file have not been updated' do
+        it 'moves the file' do
+          results = subject.execute
+          blob = project.repository.blob_at_branch(branch_name, new_file_path)
+
+          expect(results[:status]).to eq(:success)
+          expect(blob).to be_present
+        end
+      end
+    end
+
+    context 'when file status validation is skipped' do
+      let(:action) { 'create' }
+      let(:new_file_path) { 'files/ruby/new_file.rb' }
+
+      it 'does not check the last commit' do
+        expect(Gitlab::Git::Commit).not_to receive(:last_for_path)
+
+        subject.execute
+      end
+
+      it 'creates the file' do
+        subject.execute
+
+        blob = project.repository.blob_at_branch(branch_name, new_file_path)
+
+        expect(blob).to be_present
+      end
+    end
   end
 
   def update_file(path)