Commit 87f9c475 authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

Merge branch 'refactor-web-editor' into 'master'

Refactor web editor

* fix problem with editing non-master branch
* before commit make sure branch exists
* dont allow user change file in one branch and commit to another existing branch
* remove a lot of code duplication
* remove outdated statellite errors
Signed-off-by: default avatarDmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

Fixes #1761

See merge request !773
parents 01cd7d2c 7bde6ae5
...@@ -34,7 +34,7 @@ gem "browser" ...@@ -34,7 +34,7 @@ gem "browser"
# Extracting information from a git repository # Extracting information from a git repository
# Provide access to Gitlab::Git library # Provide access to Gitlab::Git library
gem "gitlab_git", '~> 7.2.2' gem "gitlab_git", '~> 7.2.3'
# Ruby/Rack Git Smart-HTTP Server Handler # Ruby/Rack Git Smart-HTTP Server Handler
# GitLab fork with a lot of changes (improved thread-safety, better memory usage etc) # GitLab fork with a lot of changes (improved thread-safety, better memory usage etc)
......
...@@ -225,7 +225,7 @@ GEM ...@@ -225,7 +225,7 @@ GEM
mime-types (~> 1.19) mime-types (~> 1.19)
gitlab_emoji (0.1.0) gitlab_emoji (0.1.0)
gemojione (~> 2.0) gemojione (~> 2.0)
gitlab_git (7.2.2) gitlab_git (7.2.3)
activesupport (~> 4.0) activesupport (~> 4.0)
charlock_holmes (~> 0.6) charlock_holmes (~> 0.6)
gitlab-linguist (~> 3.0) gitlab-linguist (~> 3.0)
...@@ -713,7 +713,7 @@ DEPENDENCIES ...@@ -713,7 +713,7 @@ DEPENDENCIES
gitlab-grack (~> 2.0.2) gitlab-grack (~> 2.0.2)
gitlab-linguist (~> 3.0.1) gitlab-linguist (~> 3.0.1)
gitlab_emoji (~> 0.1) gitlab_emoji (~> 0.1)
gitlab_git (~> 7.2.2) gitlab_git (~> 7.2.3)
gitlab_meta (= 7.0) gitlab_meta (= 7.0)
gitlab_omniauth-ldap (= 1.2.1) gitlab_omniauth-ldap (= 1.2.1)
gollum-lib (~> 4.0.2) gollum-lib (~> 4.0.2)
......
...@@ -13,27 +13,20 @@ class Projects::BlobController < Projects::ApplicationController ...@@ -13,27 +13,20 @@ class Projects::BlobController < Projects::ApplicationController
before_action :commit, except: [:new, :create] before_action :commit, except: [:new, :create]
before_action :blob, except: [:new, :create] before_action :blob, except: [:new, :create]
before_action :from_merge_request, only: [:edit, :update] before_action :from_merge_request, only: [:edit, :update]
before_action :after_edit_path, only: [:edit, :update]
before_action :require_branch_head, only: [:edit, :update] before_action :require_branch_head, only: [:edit, :update]
before_action :editor_variables, except: [:show, :preview, :diff]
before_action :after_edit_path, only: [:edit, :update]
def new def new
commit unless @repository.empty? commit unless @repository.empty?
end end
def create def create
file_path = File.join(@path, File.basename(params[:file_name])) result = Files::CreateService.new(@project, current_user, @commit_params).execute
result = Files::CreateService.new(
@project,
current_user,
params.merge(new_branch: sanitized_new_branch_name),
@ref,
file_path
).execute
if result[:status] == :success if result[:status] == :success
flash[:notice] = "Your changes have been successfully committed" flash[:notice] = "Your changes have been successfully committed"
ref = sanitized_new_branch_name.presence || @ref redirect_to namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @file_path))
redirect_to namespace_project_blob_path(@project.namespace, @project, File.join(ref, file_path))
else else
flash[:alert] = result[:message] flash[:alert] = result[:message]
render :new render :new
...@@ -48,22 +41,10 @@ class Projects::BlobController < Projects::ApplicationController ...@@ -48,22 +41,10 @@ class Projects::BlobController < Projects::ApplicationController
end end
def update def update
result = Files::UpdateService. result = Files::UpdateService.new(@project, current_user, @commit_params).execute
new(
@project,
current_user,
params.merge(new_branch: sanitized_new_branch_name),
@ref,
@path
).execute
if result[:status] == :success if result[:status] == :success
flash[:notice] = "Your changes have been successfully committed" flash[:notice] = "Your changes have been successfully committed"
if from_merge_request
from_merge_request.reload_code
end
redirect_to after_edit_path redirect_to after_edit_path
else else
flash[:alert] = result[:message] flash[:alert] = result[:message]
...@@ -80,12 +61,11 @@ class Projects::BlobController < Projects::ApplicationController ...@@ -80,12 +61,11 @@ class Projects::BlobController < Projects::ApplicationController
end end
def destroy def destroy
result = Files::DeleteService.new(@project, current_user, params, @ref, @path).execute result = Files::DeleteService.new(@project, current_user, @commit_params).execute
if result[:status] == :success if result[:status] == :success
flash[:notice] = "Your changes have been successfully committed" flash[:notice] = "Your changes have been successfully committed"
redirect_to namespace_project_tree_path(@project.namespace, @project, redirect_to namespace_project_tree_path(@project.namespace, @project, @target_branch)
@ref)
else else
flash[:alert] = result[:message] flash[:alert] = result[:message]
render :show render :show
...@@ -135,7 +115,6 @@ class Projects::BlobController < Projects::ApplicationController ...@@ -135,7 +115,6 @@ class Projects::BlobController < Projects::ApplicationController
@id = params[:id] @id = params[:id]
@ref, @path = extract_ref(@id) @ref, @path = extract_ref(@id)
rescue InvalidPathError rescue InvalidPathError
not_found! not_found!
end end
...@@ -145,8 +124,8 @@ class Projects::BlobController < Projects::ApplicationController ...@@ -145,8 +124,8 @@ class Projects::BlobController < Projects::ApplicationController
if from_merge_request if from_merge_request
diffs_namespace_project_merge_request_path(from_merge_request.target_project.namespace, from_merge_request.target_project, from_merge_request) + diffs_namespace_project_merge_request_path(from_merge_request.target_project.namespace, from_merge_request.target_project, from_merge_request) +
"#file-path-#{hexdigest(@path)}" "#file-path-#{hexdigest(@path)}"
elsif sanitized_new_branch_name.present? elsif @target_branch.present?
namespace_project_blob_path(@project.namespace, @project, File.join(sanitized_new_branch_name, @path)) namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @path))
else else
namespace_project_blob_path(@project.namespace, @project, @id) namespace_project_blob_path(@project.namespace, @project, @id)
end end
...@@ -160,4 +139,25 @@ class Projects::BlobController < Projects::ApplicationController ...@@ -160,4 +139,25 @@ class Projects::BlobController < Projects::ApplicationController
def sanitized_new_branch_name def sanitized_new_branch_name
@new_branch ||= sanitize(strip_tags(params[:new_branch])) @new_branch ||= sanitize(strip_tags(params[:new_branch]))
end end
def editor_variables
@current_branch = @ref
@target_branch = (sanitized_new_branch_name || @ref)
@file_path =
if action_name.to_s == 'create'
File.join(@path, File.basename(params[:file_name]))
else
@path
end
@commit_params = {
file_path: @file_path,
current_branch: @current_branch,
target_branch: @target_branch,
commit_message: params[:commit_message],
file_content: params[:content],
file_content_encoding: params[:encoding]
}
end
end end
module Files module Files
class BaseService < ::BaseService class BaseService < ::BaseService
attr_reader :ref, :path class ValidationError < StandardError; end
def initialize(project, user, params, ref, path = nil) def execute
@project, @current_user, @params = project, user, params.dup @current_branch = params[:current_branch]
@ref = ref @target_branch = params[:target_branch]
@path = path @commit_message = params[:commit_message]
@file_path = params[:file_path]
@file_content = if params[:file_content_encoding] == 'base64'
Base64.decode64(params[:file_content])
else
params[:file_content]
end
# Validate parameters
validate
# Create new branch if it different from current_branch
if @target_branch != @current_branch
create_target_branch
end
if sha = commit
after_commit(sha, @target_branch)
success
else
error("Something went wrong. Your changes were not committed")
end
rescue ValidationError => ex
error(ex.message)
end end
private private
...@@ -14,11 +37,51 @@ module Files ...@@ -14,11 +37,51 @@ module Files
project.repository project.repository
end end
def after_commit(sha) def after_commit(sha, branch)
commit = repository.commit(sha) commit = repository.commit(sha)
full_ref = 'refs/heads/' + (params[:new_branch] || ref) full_ref = 'refs/heads/' + branch
old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA
GitPushService.new.execute(project, current_user, old_sha, sha, full_ref) GitPushService.new.execute(project, current_user, old_sha, sha, full_ref)
end end
def current_branch
@current_branch ||= params[:current_branch]
end
def target_branch
@target_branch ||= params[:target_branch]
end
def raise_error(message)
raise ValidationError.new(message)
end
def validate
allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(@target_branch)
unless allowed
raise_error("You are not allowed to push into this branch")
end
unless project.empty_repo?
unless repository.branch_names.include?(@current_branch)
raise_error("You can only create files if you are on top of a branch")
end
if @current_branch != @target_branch
if repository.branch_names.include?(@target_branch)
raise_error("Branch with such name already exists. You need to switch to this branch in order to make changes")
end
end
end
end
def create_target_branch
result = CreateBranchService.new(project, current_user).execute(@target_branch, @current_branch)
unless result[:status] == :success
raise_error("Something went wrong when we tried to create #{@target_branch} for you")
end
end
end end
end end
...@@ -2,58 +2,28 @@ require_relative "base_service" ...@@ -2,58 +2,28 @@ require_relative "base_service"
module Files module Files
class CreateService < Files::BaseService class CreateService < Files::BaseService
def execute def commit
allowed = Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) repository.commit_file(current_user, @file_path, @file_content, @commit_message, @target_branch)
end
unless allowed def validate
return error("You are not allowed to create file in this branch") super
end
file_name = File.basename(path) file_name = File.basename(@file_path)
file_path = path
unless file_name =~ Gitlab::Regex.file_name_regex unless file_name =~ Gitlab::Regex.file_name_regex
return error( raise_error(
'Your changes could not be committed, because the file name ' + 'Your changes could not be committed, because the file name ' +
Gitlab::Regex.file_name_regex_message Gitlab::Regex.file_name_regex_message
) )
end end
if project.empty_repo? unless project.empty_repo?
# everything is ok because repo does not have a commits yet blob = repository.blob_at_branch(@current_branch, @file_path)
else
unless repository.branch_names.include?(ref)
return error("You can only create files if you are on top of a branch")
end
blob = repository.blob_at_branch(ref, file_path)
if blob if blob
return error("Your changes could not be committed, because file with such name exists") raise_error("Your changes could not be committed, because file with such name exists")
end
end
content =
if params[:encoding] == 'base64'
Base64.decode64(params[:content])
else
params[:content]
end end
sha = repository.commit_file(
current_user,
file_path,
content,
params[:commit_message],
params[:new_branch] || ref
)
if sha
after_commit(sha)
success
else
error("Your changes could not be committed, because the file has been changed")
end end
end end
end end
......
...@@ -2,36 +2,8 @@ require_relative "base_service" ...@@ -2,36 +2,8 @@ require_relative "base_service"
module Files module Files
class DeleteService < Files::BaseService class DeleteService < Files::BaseService
def execute def commit
allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) repository.remove_file(current_user, @file_path, @commit_message, @target_branch)
unless allowed
return error("You are not allowed to push into this branch")
end
unless repository.branch_names.include?(ref)
return error("You can only create files if you are on top of a branch")
end
blob = repository.blob_at_branch(ref, path)
unless blob
return error("You can only edit text files")
end
sha = repository.remove_file(
current_user,
path,
params[:commit_message],
ref
)
if sha
after_commit(sha)
success
else
error("Your changes could not be committed, because the file has been changed")
end
end end
end end
end end
...@@ -2,46 +2,8 @@ require_relative "base_service" ...@@ -2,46 +2,8 @@ require_relative "base_service"
module Files module Files
class UpdateService < Files::BaseService class UpdateService < Files::BaseService
def execute def commit
allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) repository.commit_file(current_user, @file_path, @file_content, @commit_message, @target_branch)
unless allowed
return error("You are not allowed to push into this branch")
end
unless repository.branch_names.include?(ref)
return error("You can only create files if you are on top of a branch")
end
blob = repository.blob_at_branch(ref, path)
unless blob
return error("You can only edit text files")
end
content =
if params[:encoding] == 'base64'
Base64.decode64(params[:content])
else
params[:content]
end
sha = repository.commit_file(
current_user,
path,
content,
params[:commit_message],
params[:new_branch] || ref
)
after_commit(sha)
success
rescue Gitlab::Satellite::CheckoutFailed => ex
error("Your changes could not be committed because ref '#{ref}' could not be checked out", 400)
rescue Gitlab::Satellite::CommitFailed => ex
error("Your changes could not be committed. Maybe there was nothing to commit?", 409)
rescue Gitlab::Satellite::PushFailed => ex
error("Your changes could not be committed. Maybe the file was changed by another process?", 409)
end end
end end
end end
...@@ -6,11 +6,12 @@ ...@@ -6,11 +6,12 @@
= render 'shared/commit_message_container', params: params, = render 'shared/commit_message_container', params: params,
placeholder: 'Add new file' placeholder: 'Add new file'
.form-group.branch - unless @project.empty_repo?
= label_tag 'branch', class: 'control-label' do .form-group.branch
Branch = label_tag 'branch', class: 'control-label' do
.col-sm-10 Branch
= text_field_tag 'new_branch', @ref, class: "form-control" .col-sm-10
= text_field_tag 'new_branch', @ref, class: "form-control"
= hidden_field_tag 'content', '', id: 'file-content' = hidden_field_tag 'content', '', id: 'file-content'
= render 'projects/commit_button', ref: @ref, = render 'projects/commit_button', ref: @ref,
......
...@@ -3,6 +3,26 @@ module API ...@@ -3,6 +3,26 @@ module API
class Files < Grape::API class Files < Grape::API
before { authenticate! } before { authenticate! }
helpers do
def commit_params(attrs)
{
file_path: attrs[:file_path],
current_branch: attrs[:branch_name],
target_branch: attrs[:branch_name],
commit_message: attrs[:commit_message],
file_content: attrs[:content],
file_content_encoding: attrs[:encoding]
}
end
def commit_response(attrs)
{
file_path: attrs[:file_path],
branch_name: attrs[:branch_name],
}
end
end
resource :projects do resource :projects do
# Get file from repository # Get file from repository
# File content is Base64 encoded # File content is Base64 encoded
...@@ -73,17 +93,11 @@ module API ...@@ -73,17 +93,11 @@ module API
required_attributes! [:file_path, :branch_name, :content, :commit_message] required_attributes! [:file_path, :branch_name, :content, :commit_message]
attrs = attributes_for_keys [:file_path, :branch_name, :content, :commit_message, :encoding] attrs = attributes_for_keys [:file_path, :branch_name, :content, :commit_message, :encoding]
branch_name = attrs.delete(:branch_name) result = ::Files::CreateService.new(user_project, current_user, commit_params(attrs)).execute
file_path = attrs.delete(:file_path)
result = ::Files::CreateService.new(user_project, current_user, attrs, branch_name, file_path).execute
if result[:status] == :success if result[:status] == :success
status(201) status(201)
commit_response(attrs)
{
file_path: file_path,
branch_name: branch_name
}
else else
render_api_error!(result[:message], 400) render_api_error!(result[:message], 400)
end end
...@@ -105,17 +119,11 @@ module API ...@@ -105,17 +119,11 @@ module API
required_attributes! [:file_path, :branch_name, :content, :commit_message] required_attributes! [:file_path, :branch_name, :content, :commit_message]
attrs = attributes_for_keys [:file_path, :branch_name, :content, :commit_message, :encoding] attrs = attributes_for_keys [:file_path, :branch_name, :content, :commit_message, :encoding]
branch_name = attrs.delete(:branch_name) result = ::Files::UpdateService.new(user_project, current_user, commit_params(attrs)).execute
file_path = attrs.delete(:file_path)
result = ::Files::UpdateService.new(user_project, current_user, attrs, branch_name, file_path).execute
if result[:status] == :success if result[:status] == :success
status(200) status(200)
commit_response(attrs)
{
file_path: file_path,
branch_name: branch_name
}
else else
http_status = result[:http_status] || 400 http_status = result[:http_status] || 400
render_api_error!(result[:message], http_status) render_api_error!(result[:message], http_status)
...@@ -138,17 +146,11 @@ module API ...@@ -138,17 +146,11 @@ module API
required_attributes! [:file_path, :branch_name, :commit_message] required_attributes! [:file_path, :branch_name, :commit_message]
attrs = attributes_for_keys [:file_path, :branch_name, :commit_message] attrs = attributes_for_keys [:file_path, :branch_name, :commit_message]
branch_name = attrs.delete(:branch_name) result = ::Files::DeleteService.new(user_project, current_user, commit_params(attrs)).execute
file_path = attrs.delete(:file_path)
result = ::Files::DeleteService.new(user_project, current_user, attrs, branch_name, file_path).execute
if result[:status] == :success if result[:status] == :success
status(200) status(200)
commit_response(attrs)
{
file_path: file_path,
branch_name: branch_name
}
else else
render_api_error!(result[:message], 400) render_api_error!(result[:message], 400)
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment