Merge branch 'frank-west-iii/gitlab-ce-fwiii-5857-web-editor-overwrites-commits'

CHANGELOG

@@ -114,6 +114,7 @@ v 8.11.0 (unreleased)
   - Add auto-completition in pipeline (Katarzyna Kobierska Ula Budziszewska)
   - Fix a memory leak caused by Banzai::Filter::SanitizationFilter
   - Speed up todos queries by limiting the projects set we join with
+  - Ensure file editing in UI does not overwrite commited changes without warning user
 
 v 8.10.5
   - Add a data migration to fix some missing timestamps in the members table. !5670

app/controllers/projects/blob_controller.rb

@@ -17,6 +17,7 @@ class Projects::BlobController < Projects::ApplicationController
   before_action :require_branch_head, only: [:edit, :update]
   before_action :editor_variables, except: [:show, :preview, :diff]
   before_action :validate_diff_params, only: :diff
+  before_action :set_last_commit_sha, only: [:edit, :update]
 
   def new
     commit unless @repository.empty?
@@ -33,7 +34,6 @@ class Projects::BlobController < Projects::ApplicationController
   end
 
   def edit
-    @last_commit = Gitlab::Git::Commit.last_for_path(@repository, @ref, @path).sha
     blob.load_all_data!(@repository)
   end
 
@@ -55,6 +55,10 @@ class Projects::BlobController < Projects::ApplicationController
     create_commit(Files::UpdateService, success_path: after_edit_path,
                                         failure_view: :edit,
                                         failure_path: namespace_project_blob_path(@project.namespace, @project, @id))
+
+  rescue Files::UpdateService::FileChangedError
+    @conflict = true
+    render :edit
   end
 
   def preview
@@ -152,7 +156,8 @@ class Projects::BlobController < Projects::ApplicationController
       file_path: @file_path,
       commit_message: params[:commit_message],
       file_content: params[:content],
-      file_content_encoding: params[:encoding]
+      file_content_encoding: params[:encoding],
+      last_commit_sha: params[:last_commit_sha]
     }
   end
 
@@ -161,4 +166,9 @@ class Projects::BlobController < Projects::ApplicationController
       render nothing: true
     end
   end
+
+  def set_last_commit_sha
+    @last_commit_sha = Gitlab::Git::Commit.
+      last_for_path(@repository, @ref, @path).sha
+  end
 end

app/services/files/base_service.rb

@@ -15,6 +15,7 @@ module Files
                         else
                           params[:file_content]
                         end
+      @last_commit_sha = params[:last_commit_sha]
 
       # Validate parameters
       validate

app/services/files/update_service.rb

@@ -2,11 +2,34 @@ require_relative "base_service"
 
 module Files
   class UpdateService < Files::BaseService
+    class FileChangedError < StandardError; end
+
     def commit
       repository.update_file(current_user, @file_path, @file_content,
                              branch: @target_branch,
                              previous_path: @previous_path,
                              message: @commit_message)
     end
+
+    private
+
+    def validate
+      super
+
+      if file_has_changed?
+        raise FileChangedError.new("You are attempting to update a file that has changed since you started editing it.")
+      end
+    end
+
+    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(@source_project.repository, @source_branch, @file_path)
+    end
   end
 end

app/views/projects/blob/edit.html.haml

 - page_title "Edit", @blob.path, @ref
 
+- if @conflict
+  .alert.alert-danger
+    Someone edited the file the same time you did. Please check out
+    = link_to "the file", namespace_project_blob_path(@project.namespace, @project, tree_join(@target_branch, @file_path)), target: "_blank"
+    and make sure your changes will not unintentionally remove theirs.
+
 .file-editor
   %ul.nav-links.no-bottom.js-edit-mode
     %li.active
@@ -13,8 +19,7 @@
   = form_tag(namespace_project_update_blob_path(@project.namespace, @project, @id), method: :put, class: 'form-horizontal js-quick-submit js-requires-input js-edit-blob-form') do
     = render 'projects/blob/editor', ref: @ref, path: @path, blob_data: @blob.data
     = render 'shared/new_commit_form', placeholder: "Update #{@blob.name}"
-
-    = hidden_field_tag 'last_commit', @last_commit
+    = hidden_field_tag 'last_commit_sha', @last_commit_sha
     = hidden_field_tag 'content', '', id: "file-content"
     = hidden_field_tag 'from_merge_request_id', params[:from_merge_request_id]
     = render 'projects/commit_button', ref: @ref, cancel_path: namespace_project_blob_path(@project.namespace, @project, @id)

spec/features/projects/files/editing_a_file_spec.rb

+require 'spec_helper'
+
+feature 'User wants to edit a file', feature: true do
+  include WaitForAjax
+
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+  let(:commit_params) do
+    {
+      source_branch: project.default_branch,
+      target_branch: project.default_branch,
+      commit_message: "Committing First Update",
+      file_path: ".gitignore",
+      file_content: "First Update",
+      last_commit_sha: Gitlab::Git::Commit.last_for_path(project.repository, project.default_branch,
+                                                         ".gitignore").sha
+    }
+  end
+
+  background do
+    project.team << [user, :master]
+    login_as user
+    visit namespace_project_edit_blob_path(project.namespace, project,
+                                           File.join(project.default_branch, '.gitignore'))
+  end
+
+  scenario 'file has been updated since the user opened the edit page' do
+    Files::UpdateService.new(project, user, commit_params).execute
+
+    click_button 'Commit Changes'
+
+    expect(page).to have_content 'Someone edited the file the same time you did.'
+  end
+end

spec/services/files/update_service_spec.rb

+require "spec_helper"
+
+describe Files::UpdateService do
+  subject { described_class.new(project, user, commit_params) }
+
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+  let(:file_path) { 'files/ruby/popen.rb' }
+  let(:new_contents) { "New Content" }
+  let(:commit_params) do
+    {
+      file_path: file_path,
+      commit_message: "Update File",
+      file_content: new_contents,
+      file_content_encoding: "text",
+      last_commit_sha: last_commit_sha,
+      source_project: project,
+      source_branch: project.default_branch,
+      target_branch: project.default_branch,
+    }
+  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 update a file that has changed since you started editing it.")
+      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 "returns a hash with the :success status " do
+        results = subject.execute
+
+        expect(results).to match({ status: :success })
+      end
+
+      it "updates the file with the new contents" do
+        subject.execute
+
+        results = project.repository.blob_at_branch(project.default_branch, file_path)
+
+        expect(results.data).to eq(new_contents)
+      end
+    end
+
+    context "when the last_commit_sha is not supplied" do
+      let(:commit_params) do
+        {
+          file_path: file_path,
+          commit_message: "Update File",
+          file_content: new_contents,
+          file_content_encoding: "text",
+          source_project: project,
+          source_branch: project.default_branch,
+          target_branch: project.default_branch,
+        }
+      end
+
+      it "returns a hash with the :success status " do
+        results = subject.execute
+
+        expect(results).to match({ status: :success })
+      end
+
+      it "updates the file with the new contents" do
+        subject.execute
+
+        results = project.repository.blob_at_branch(project.default_branch, file_path)
+
+        expect(results.data).to eq(new_contents)
+      end
+    end
+  end
+end