diff --git a/CHANGELOG b/CHANGELOG index 993b229f975..616f006b29e 100644 --- a/CHANGELOG +++ b/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 diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 19d051720e9..cdf9a04bacf 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/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 diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index c4a206f785e..ea94818713b 100644 --- a/app/services/files/base_service.rb +++ b/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 diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb index 8d2b5083179..4fc3b640799 100644 --- a/app/services/files/update_service.rb +++ b/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 diff --git a/app/views/projects/blob/edit.html.haml b/app/views/projects/blob/edit.html.haml index b1c9895f43e..7b0621f9401 100644 --- a/app/views/projects/blob/edit.html.haml +++ b/app/views/projects/blob/edit.html.haml @@ -1,5 +1,11 @@ - 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) diff --git a/spec/features/projects/files/editing_a_file_spec.rb b/spec/features/projects/files/editing_a_file_spec.rb new file mode 100644 index 00000000000..fe047e00409 --- /dev/null +++ b/spec/features/projects/files/editing_a_file_spec.rb @@ -0,0 +1,34 @@ +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 diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb new file mode 100644 index 00000000000..d019e50649f --- /dev/null +++ b/spec/services/files/update_service_spec.rb @@ -0,0 +1,84 @@ +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