diff options
author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2013-12-12 13:10:04 +0000 |
---|---|---|
committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2013-12-12 13:10:04 +0000 |
commit | 03e517e0776f65aefd659958841e37c5c3f82a64 (patch) | |
tree | bc30e3eab64ad905a8d3f9b1c390b04f9fabdb3a | |
parent | 7bb2a536a02454bef0f5b9c6def342992128da36 (diff) | |
parent | 8c5aa5e3b1e9cae4962b797c15cb3366cb689b38 (diff) | |
download | gitlab-ce-03e517e0776f65aefd659958841e37c5c3f82a64.tar.gz |
Merge branch 'bug/mr_duplicate_iids' of /home/git/repositories/gitlab/gitlabhq
-rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 26 | ||||
-rw-r--r-- | app/helpers/merge_requests_helper.rb | 2 | ||||
-rw-r--r-- | app/models/merge_request.rb | 36 | ||||
-rw-r--r-- | app/models/project.rb | 10 | ||||
-rw-r--r-- | app/views/projects/merge_requests/_form.html.haml | 2 | ||||
-rw-r--r-- | app/views/projects/merge_requests/_merge_request.html.haml | 2 | ||||
-rw-r--r-- | app/views/projects/merge_requests/_show.html.haml | 5 | ||||
-rw-r--r-- | app/views/projects/merge_requests/invalid.html.haml | 24 | ||||
-rw-r--r-- | app/views/projects/merge_requests/show/_how_to_merge.html.haml | 2 | ||||
-rw-r--r-- | app/views/projects/merge_requests/show/_mr_title.html.haml | 4 | ||||
-rw-r--r-- | app/views/projects/merge_requests/show/_no_accept.html.haml | 12 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 2 |
12 files changed, 101 insertions, 26 deletions
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 2f285f8ba85..7d7c1104ec9 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -79,6 +79,21 @@ class Projects::MergeRequestsController < Projects::ApplicationController end def update + # If we close MergeRequest we want to ignore validation + # so we can close broken one (Ex. fork project removed) + if params[:merge_request] == {"state_event"=>"close"} + @merge_request.allow_broken = true + + if @merge_request.close + opts = { notice: 'Merge request was successfully closed.' } + else + opts = { alert: 'Failed to close merge request.' } + end + + redirect_to [@merge_request.target_project, @merge_request], opts + return + end + if @merge_request.update_attributes(params[:merge_request].merge(author_id_of_changes: current_user.id)) @merge_request.reload_code @merge_request.mark_as_unchecked @@ -160,14 +175,17 @@ class Projects::MergeRequestsController < Projects::ApplicationController end def validates_merge_request + # If source project was removed (Ex. mr from fork to origin) + return invalid_mr unless @merge_request.source_project + # Show git not found page # if there is no saved commits between source & target branch if @merge_request.commits.blank? - # and if source target doesn't exist - return invalid_mr unless @merge_request.target_project.repository.branch_names.include?(@merge_request.target_branch) + # and if target branch doesn't exist + return invalid_mr unless @merge_request.target_branch_exists? - # or if source branch doesn't exist - return invalid_mr unless @merge_request.source_project.repository.branch_names.include?(@merge_request.source_branch) + # or if source branch doesn't exist + return invalid_mr unless @merge_request.source_branch_exists? end end diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 4ba48aa4339..5e3f82fe9ce 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -36,7 +36,7 @@ module MergeRequestsHelper def merge_path_description(merge_request, separator) if merge_request.for_fork? - "Project:Branches: #{@merge_request.source_project.path_with_namespace}:#{@merge_request.source_branch} #{separator} #{@merge_request.target_project.path_with_namespace}:#{@merge_request.target_branch}" + "Project:Branches: #{@merge_request.source_project_path}:#{@merge_request.source_branch} #{separator} #{@merge_request.target_project.path_with_namespace}:#{@merge_request.target_branch}" else "Branches: #{@merge_request.source_branch} #{separator} #{@merge_request.target_branch}" end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b164ea11073..e862f35819c 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -35,6 +35,10 @@ class MergeRequest < ActiveRecord::Base attr_accessor :should_remove_source_branch + # When this attribute is true some MR validation is ignored + # It allows us to close or modify broken merge requests + attr_accessor :allow_broken + state_machine :state, initial: :opened do event :close do transition [:reopened, :opened] => :closed @@ -80,7 +84,7 @@ class MergeRequest < ActiveRecord::Base serialize :st_commits serialize :st_diffs - validates :source_project, presence: true + validates :source_project, presence: true, unless: :allow_broken validates :source_branch, presence: true validates :target_project, presence: true validates :target_branch, presence: true @@ -262,7 +266,7 @@ class MergeRequest < ActiveRecord::Base # Return the set of issues that will be closed if this merge request is accepted. def closes_issues if target_branch == project.default_branch - unmerged_commits.map { |c| c.closes_issues(project) }.flatten.uniq.sort_by(&:id) + commits.map { |c| c.closes_issues(project) }.flatten.uniq.sort_by(&:id) else [] end @@ -273,6 +277,34 @@ class MergeRequest < ActiveRecord::Base "merge request !#{iid}" end + def target_project_path + if target_project + target_project.path_with_namespace + else + "(removed)" + end + end + + def source_project_path + if source_project + source_project.path_with_namespace + else + "(removed)" + end + end + + def source_branch_exists? + return false unless self.source_project + + self.source_project.repository.branch_names.include?(self.source_branch) + end + + def target_branch_exists? + return false unless self.target_project + + self.target_project.repository.branch_names.include?(self.target_branch) + end + private def dump_commits(commits) diff --git a/app/models/project.rb b/app/models/project.rb index 48fa2f66170..39f0d71b337 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -55,11 +55,15 @@ class Project < ActiveRecord::Base has_one :forked_project_link, dependent: :destroy, foreign_key: "forked_to_project_id" has_one :forked_from_project, through: :forked_project_link - has_many :services, dependent: :destroy - has_many :events, dependent: :destroy + # Merge Requests for target project should be removed with it has_many :merge_requests, dependent: :destroy, foreign_key: "target_project_id" - has_many :fork_merge_requests,dependent: :destroy, foreign_key: "source_project_id", class_name: MergeRequest + + # Merge requests from source project should be kept when source project was removed + has_many :fork_merge_requests, foreign_key: "source_project_id", class_name: MergeRequest + has_many :issues, -> { order "state DESC, created_at DESC" }, dependent: :destroy + has_many :services, dependent: :destroy + has_many :events, dependent: :destroy has_many :milestones, dependent: :destroy has_many :notes, dependent: :destroy has_many :snippets, dependent: :destroy, class_name: "ProjectSnippet" diff --git a/app/views/projects/merge_requests/_form.html.haml b/app/views/projects/merge_requests/_form.html.haml index c78be4965d0..abb8a5a3787 100644 --- a/app/views/projects/merge_requests/_form.html.haml +++ b/app/views/projects/merge_requests/_form.html.haml @@ -10,7 +10,7 @@ .span5 .clearfix .pull-left - = f.select(:source_project_id,[[@merge_request.source_project.path_with_namespace,@merge_request.source_project.id]] , {}, {class: 'source_project chosen span3'}) + = f.select(:source_project_id,[[@merge_request.source_project_path,@merge_request.source_project.id]] , {}, {class: 'source_project chosen span3'}) .pull-left = f.select(:source_branch, @merge_request.source_project.repository.branch_names, { include_blank: "Select branch" }, {class: 'source_branch chosen span2'}) diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml index 933d78bcbfb..faa67e1aeb0 100644 --- a/app/views/projects/merge_requests/_merge_request.html.haml +++ b/app/views/projects/merge_requests/_merge_request.html.haml @@ -10,7 +10,7 @@ %span.pull-right - if merge_request.for_fork? %span.light - = "#{merge_request.source_project.path_with_namespace}" + = "#{merge_request.source_project_path}" = "#{merge_request.source_branch}" %i.icon-angle-right.light = "#{merge_request.target_branch}" diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index 47c14abdedd..f4852ffef8b 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -2,7 +2,10 @@ = render "projects/merge_requests/show/mr_title" = render "projects/merge_requests/show/how_to_merge" = render "projects/merge_requests/show/mr_box" - = render "projects/merge_requests/show/mr_accept" + - if @merge_request.source_branch_exists? && @merge_request.target_branch_exists? + = render "projects/merge_requests/show/mr_accept" + - else + = render "projects/merge_requests/show/no_accept" - if @merge_request.source_project.gitlab_ci? = render "projects/merge_requests/show/mr_ci" = render "projects/merge_requests/show/commits" diff --git a/app/views/projects/merge_requests/invalid.html.haml b/app/views/projects/merge_requests/invalid.html.haml index c962811a3e4..16e9fee42a2 100644 --- a/app/views/projects/merge_requests/invalid.html.haml +++ b/app/views/projects/merge_requests/invalid.html.haml @@ -3,15 +3,21 @@ = render "projects/merge_requests/show/mr_box" .alert.alert-error - %h5 - %i.icon-exclamation-sign - We cannot find - %span.label-branch= @merge_request.source_branch - or - %span.label-branch= @merge_request.target_branch - branches in the repository. - %p - Maybe it was removed or never pushed. %p + We cannot render this merge request properly because + - if @merge_request.for_fork? && !@merge_request.source_project + fork project was removed + - elsif !@merge_request.source_branch_exists? + %span.label.label-inverse= @merge_request.source_branch + does not exist in + %span.label.label-info= @merge_request.source_project_path + - elsif !@merge_request.target_branch_exists? + %span.label.label-inverse= @merge_request.target_branch + does not exist in + %span.label.label-info= @merge_request.target_project_path + - else + of internal error + + %strong Please close Merge Request or change branches with existing one diff --git a/app/views/projects/merge_requests/show/_how_to_merge.html.haml b/app/views/projects/merge_requests/show/_how_to_merge.html.haml index 030ac285f2a..98d373e8ad4 100644 --- a/app/views/projects/merge_requests/show/_how_to_merge.html.haml +++ b/app/views/projects/merge_requests/show/_how_to_merge.html.haml @@ -10,7 +10,7 @@ %strong Step 1. Checkout target branch and get recent objects from GitLab Assuming remote for #{@merge_request.target_project.path_with_namespace} is called #{target_remote} - remote for #{@merge_request.source_project.path_with_namespace} is called #{source_remote} + remote for #{@merge_request.source_project_path} is called #{source_remote} %pre.dark :preserve git checkout #{target_remote} #{@merge_request.target_branch} diff --git a/app/views/projects/merge_requests/show/_mr_title.html.haml b/app/views/projects/merge_requests/show/_mr_title.html.haml index 096a9167645..456101fb5ee 100644 --- a/app/views/projects/merge_requests/show/_mr_title.html.haml +++ b/app/views/projects/merge_requests/show/_mr_title.html.haml @@ -3,7 +3,7 @@ -if @merge_request.for_fork? %span.label-branch - %span.label-project= truncate(@merge_request.source_project.path_with_namespace, length: 25) + %span.label-project= truncate(@merge_request.source_project_path, length: 25) #{@merge_request.source_branch} → %span.label-branch= @merge_request.target_branch @@ -24,7 +24,7 @@ %li= link_to "Email Patches", project_merge_request_path(@project, @merge_request, format: :patch) %li= link_to "Plain Diff", project_merge_request_path(@project, @merge_request, format: :diff) - = link_to 'Close', project_merge_request_path(@project, @merge_request, merge_request: {state_event: :close }), method: :put, class: "btn grouped btn-close", title: "Close merge request" + = link_to 'Close', project_merge_request_path(@project, @merge_request, merge_request: { state_event: :close }), method: :put, class: "btn grouped btn-close", title: "Close merge request" = link_to edit_project_merge_request_path(@project, @merge_request), class: "btn grouped", id:"edit_merge_request" do %i.icon-edit diff --git a/app/views/projects/merge_requests/show/_no_accept.html.haml b/app/views/projects/merge_requests/show/_no_accept.html.haml new file mode 100644 index 00000000000..a0507b24ad8 --- /dev/null +++ b/app/views/projects/merge_requests/show/_no_accept.html.haml @@ -0,0 +1,12 @@ +.alert.alert-error + %p + This merge request can not be accepted because branch + - unless @merge_request.source_branch_exists? + %span.label.label-inverse= @merge_request.source_branch + does not exist in + %span.label.label-info= @merge_request.source_project_path + - else + %span.label.label-inverse= @merge_request.target_branch + does not exist in + %span.label.label-info= @merge_request.target_project_path + %strong Please close this merge request or change branches with existing one diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index fe65096fd1e..3a524158a47 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -112,7 +112,7 @@ describe MergeRequest do let(:commit2) { mock('commit2', closes_issues: [issue1]) } before do - subject.stub(unmerged_commits: [commit0, commit1, commit2]) + subject.stub(commits: [commit0, commit1, commit2]) end it 'accesses the set of issues that will be closed on acceptance' do |