Merge branch 'bug/mr_duplicate_iids' of /home/git/repositories/gitlab/gitlabhq

Dmitriy Zaporozhets
2 parents 7bb2a536 8c5aa5e3
Showing 12 changed files with 101 additions and 26 deletions Show diff stats
app/controllers/projects/merge_requests_controller.rb
app/helpers/merge_requests_helper.rb
app/models/merge_request.rb
app/models/project.rb
app/views/projects/merge_requests/_form.html.haml
app/views/projects/merge_requests/_merge_request.html.haml
app/views/projects/merge_requests/_show.html.haml
app/views/projects/merge_requests/invalid.html.haml
app/views/projects/merge_requests/show/_how_to_merge.html.haml
app/views/projects/merge_requests/show/_mr_title.html.haml
app/views/projects/merge_requests/show/_no_accept.html.haml
spec/models/merge_request_spec.rb
@@ -79,6 +79,21 @@ class Projects::MergeRequestsController &lt; 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 &lt; 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
@@ -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
@@ -35,6 +35,10 @@ class MergeRequest &lt; 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 &lt; 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 &lt; 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 &lt; 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)
@@ -55,11 +55,15 @@ class Project &lt; 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"
@@ -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
             &nbsp;
             = f.select(:source_branch, @merge_request.source_project.repository.branch_names, { include_blank: "Select branch" }, {class: 'source_branch chosen span2'})
@@ -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}"
@@ -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"
@@ -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
@@ -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}
@@ -3,7 +3,7 @@
   &nbsp;
   -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}
     &rarr;
     %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
@@ -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
@@ -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
	@@ -79,6 +79,21 @@ class Projects::MergeRequestsController < Projects::ApplicationController		@@ -79,6 +79,21 @@ class Projects::MergeRequestsController < Projects::ApplicationController
79	end	79	end
80		80
81	def update	81	def update
		82	+ # If we close MergeRequest we want to ignore validation
		83	+ # so we can close broken one (Ex. fork project removed)
		84	+ if params[:merge_request] == {"state_event"=>"close"}
		85	+ @merge_request.allow_broken = true
		86	+
		87	+ if @merge_request.close
		88	+ opts = { notice: 'Merge request was successfully closed.' }
		89	+ else
		90	+ opts = { alert: 'Failed to close merge request.' }
		91	+ end
		92	+
		93	+ redirect_to [@merge_request.target_project, @merge_request], opts
		94	+ return
		95	+ end
		96	+
82	if @merge_request.update_attributes(params[:merge_request].merge(author_id_of_changes: current_user.id))	97	if @merge_request.update_attributes(params[:merge_request].merge(author_id_of_changes: current_user.id))
83	@merge_request.reload_code	98	@merge_request.reload_code
84	@merge_request.mark_as_unchecked	99	@merge_request.mark_as_unchecked
	@@ -160,14 +175,17 @@ class Projects::MergeRequestsController < Projects::ApplicationController		@@ -160,14 +175,17 @@ class Projects::MergeRequestsController < Projects::ApplicationController
160	end	175	end
161		176
162	def validates_merge_request	177	def validates_merge_request
		178	+ # If source project was removed (Ex. mr from fork to origin)
		179	+ return invalid_mr unless @merge_request.source_project
		180	+
163	# Show git not found page	181	# Show git not found page
164	# if there is no saved commits between source & target branch	182	# if there is no saved commits between source & target branch
165	if @merge_request.commits.blank?	183	if @merge_request.commits.blank?
166	- # and if source target doesn't exist
167	- return invalid_mr unless @merge_request.target_project.repository.branch_names.include?(@merge_request.target_branch)	184	+ # and if target branch doesn't exist
		185	+ return invalid_mr unless @merge_request.target_branch_exists?
168		186
169	- # or if source branch doesn't exist
170	- return invalid_mr unless @merge_request.source_project.repository.branch_names.include?(@merge_request.source_branch)	187	+ # or if source branch doesn't exist
		188	+ return invalid_mr unless @merge_request.source_branch_exists?
171	end	189	end
172	end	190	end
173		191
	@@ -36,7 +36,7 @@ module MergeRequestsHelper		@@ -36,7 +36,7 @@ module MergeRequestsHelper
36		36
37	def merge_path_description(merge_request, separator)	37	def merge_path_description(merge_request, separator)
38	if merge_request.for_fork?	38	if merge_request.for_fork?
39	- "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}"	39	+ "Project:Branches: #{@merge_request.source_project_path}:#{@merge_request.source_branch} #{separator} #{@merge_request.target_project.path_with_namespace}:#{@merge_request.target_branch}"
40	else	40	else
41	"Branches: #{@merge_request.source_branch} #{separator} #{@merge_request.target_branch}"	41	"Branches: #{@merge_request.source_branch} #{separator} #{@merge_request.target_branch}"
42	end	42	end
	@@ -35,6 +35,10 @@ class MergeRequest < ActiveRecord::Base		@@ -35,6 +35,10 @@ class MergeRequest < ActiveRecord::Base
35		35
36	attr_accessor :should_remove_source_branch	36	attr_accessor :should_remove_source_branch
37		37
		38	+ # When this attribute is true some MR validation is ignored
		39	+ # It allows us to close or modify broken merge requests
		40	+ attr_accessor :allow_broken
		41	+
38	state_machine :state, initial: :opened do	42	state_machine :state, initial: :opened do
39	event :close do	43	event :close do
40	transition [:reopened, :opened] => :closed	44	transition [:reopened, :opened] => :closed
	@@ -80,7 +84,7 @@ class MergeRequest < ActiveRecord::Base		@@ -80,7 +84,7 @@ class MergeRequest < ActiveRecord::Base
80	serialize :st_commits	84	serialize :st_commits
81	serialize :st_diffs	85	serialize :st_diffs
82		86
83	- validates :source_project, presence: true	87	+ validates :source_project, presence: true, unless: :allow_broken
84	validates :source_branch, presence: true	88	validates :source_branch, presence: true
85	validates :target_project, presence: true	89	validates :target_project, presence: true
86	validates :target_branch, presence: true	90	validates :target_branch, presence: true
	@@ -262,7 +266,7 @@ class MergeRequest < ActiveRecord::Base		@@ -262,7 +266,7 @@ class MergeRequest < ActiveRecord::Base
262	# Return the set of issues that will be closed if this merge request is accepted.	266	# Return the set of issues that will be closed if this merge request is accepted.
263	def closes_issues	267	def closes_issues
264	if target_branch == project.default_branch	268	if target_branch == project.default_branch
265	- unmerged_commits.map { \|c\| c.closes_issues(project) }.flatten.uniq.sort_by(&:id)	269	+ commits.map { \|c\| c.closes_issues(project) }.flatten.uniq.sort_by(&:id)
266	else	270	else
267	[]	271	[]
268	end	272	end
	@@ -273,6 +277,34 @@ class MergeRequest < ActiveRecord::Base		@@ -273,6 +277,34 @@ class MergeRequest < ActiveRecord::Base
273	"merge request !#{iid}"	277	"merge request !#{iid}"
274	end	278	end
275		279
		280	+ def target_project_path
		281	+ if target_project
		282	+ target_project.path_with_namespace
		283	+ else
		284	+ "(removed)"
		285	+ end
		286	+ end
		287	+
		288	+ def source_project_path
		289	+ if source_project
		290	+ source_project.path_with_namespace
		291	+ else
		292	+ "(removed)"
		293	+ end
		294	+ end
		295	+
		296	+ def source_branch_exists?
		297	+ return false unless self.source_project
		298	+
		299	+ self.source_project.repository.branch_names.include?(self.source_branch)
		300	+ end
		301	+
		302	+ def target_branch_exists?
		303	+ return false unless self.target_project
		304	+
		305	+ self.target_project.repository.branch_names.include?(self.target_branch)
		306	+ end
		307	+
276	private	308	private
277		309
278	def dump_commits(commits)	310	def dump_commits(commits)
	@@ -10,7 +10,7 @@		@@ -10,7 +10,7 @@
10	.span5	10	.span5
11	.clearfix	11	.clearfix
12	.pull-left	12	.pull-left
13	- = f.select(:source_project_id,[[@merge_request.source_project.path_with_namespace,@merge_request.source_project.id]] , {}, {class: 'source_project chosen span3'})	13	+ = f.select(:source_project_id,[[@merge_request.source_project_path,@merge_request.source_project.id]] , {}, {class: 'source_project chosen span3'})
14	.pull-left	14	.pull-left
15		15
16	= f.select(:source_branch, @merge_request.source_project.repository.branch_names, { include_blank: "Select branch" }, {class: 'source_branch chosen span2'})	16	= f.select(:source_branch, @merge_request.source_project.repository.branch_names, { include_blank: "Select branch" }, {class: 'source_branch chosen span2'})
	@@ -10,7 +10,7 @@		@@ -10,7 +10,7 @@
10	%span.pull-right	10	%span.pull-right
11	- if merge_request.for_fork?	11	- if merge_request.for_fork?
12	%span.light	12	%span.light
13	- = "#{merge_request.source_project.path_with_namespace}"	13	+ = "#{merge_request.source_project_path}"
14	= "#{merge_request.source_branch}"	14	= "#{merge_request.source_branch}"
15	%i.icon-angle-right.light	15	%i.icon-angle-right.light
16	= "#{merge_request.target_branch}"	16	= "#{merge_request.target_branch}"
	@@ -3,15 +3,21 @@		@@ -3,15 +3,21 @@
3	= render "projects/merge_requests/show/mr_box"	3	= render "projects/merge_requests/show/mr_box"
4		4
5	.alert.alert-error	5	.alert.alert-error
6	- %h5
7	- %i.icon-exclamation-sign
8	- We cannot find
9	- %span.label-branch= @merge_request.source_branch
10	- or
11	- %span.label-branch= @merge_request.target_branch
12	- branches in the repository.
13	- %p
14	- Maybe it was removed or never pushed.
15	%p	6	%p
		7	+ We cannot render this merge request properly because
		8	+ - if @merge_request.for_fork? && !@merge_request.source_project
		9	+ fork project was removed
		10	+ - elsif !@merge_request.source_branch_exists?
		11	+ %span.label.label-inverse= @merge_request.source_branch
		12	+ does not exist in
		13	+ %span.label.label-info= @merge_request.source_project_path
		14	+ - elsif !@merge_request.target_branch_exists?
		15	+ %span.label.label-inverse= @merge_request.target_branch
		16	+ does not exist in
		17	+ %span.label.label-info= @merge_request.target_project_path
		18	+ - else
		19	+ of internal error
		20	+
		21	+ %strong
16	Please close Merge Request or change branches with existing one	22	Please close Merge Request or change branches with existing one
17		23
	@@ -10,7 +10,7 @@		@@ -10,7 +10,7 @@
10	%strong Step 1.	10	%strong Step 1.
11	Checkout target branch and get recent objects from GitLab	11	Checkout target branch and get recent objects from GitLab
12	Assuming remote for #{@merge_request.target_project.path_with_namespace} is called #{target_remote}	12	Assuming remote for #{@merge_request.target_project.path_with_namespace} is called #{target_remote}
13	- remote for #{@merge_request.source_project.path_with_namespace} is called #{source_remote}	13	+ remote for #{@merge_request.source_project_path} is called #{source_remote}
14	%pre.dark	14	%pre.dark
15	:preserve	15	:preserve
16	git checkout #{target_remote} #{@merge_request.target_branch}	16	git checkout #{target_remote} #{@merge_request.target_branch}
	@@ -3,7 +3,7 @@		@@ -3,7 +3,7 @@
3		3
4	-if @merge_request.for_fork?	4	-if @merge_request.for_fork?
5	%span.label-branch	5	%span.label-branch
6	- %span.label-project= truncate(@merge_request.source_project.path_with_namespace, length: 25)	6	+ %span.label-project= truncate(@merge_request.source_project_path, length: 25)
7	#{@merge_request.source_branch}	7	#{@merge_request.source_branch}
8	→	8	→
9	%span.label-branch= @merge_request.target_branch	9	%span.label-branch= @merge_request.target_branch
	@@ -24,7 +24,7 @@		@@ -24,7 +24,7 @@
24	%li= link_to "Email Patches", project_merge_request_path(@project, @merge_request, format: :patch)	24	%li= link_to "Email Patches", project_merge_request_path(@project, @merge_request, format: :patch)
25	%li= link_to "Plain Diff", project_merge_request_path(@project, @merge_request, format: :diff)	25	%li= link_to "Plain Diff", project_merge_request_path(@project, @merge_request, format: :diff)
26		26
27	- = 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"	27	+ = 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"
28		28
29	= link_to edit_project_merge_request_path(@project, @merge_request), class: "btn grouped", id:"edit_merge_request" do	29	= link_to edit_project_merge_request_path(@project, @merge_request), class: "btn grouped", id:"edit_merge_request" do
30	%i.icon-edit	30	%i.icon-edit
@@ -0,0 +1,12 @@		@@ -0,0 +1,12 @@
	1	+.alert.alert-error
	2	+ %p
	3	+ This merge request can not be accepted because branch
	4	+ - unless @merge_request.source_branch_exists?
	5	+ %span.label.label-inverse= @merge_request.source_branch
	6	+ does not exist in
	7	+ %span.label.label-info= @merge_request.source_project_path
	8	+ - else
	9	+ %span.label.label-inverse= @merge_request.target_branch
	10	+ does not exist in
	11	+ %span.label.label-info= @merge_request.target_project_path
	12	+ %strong Please close this merge request or change branches with existing one
	@@ -112,7 +112,7 @@ describe MergeRequest do		@@ -112,7 +112,7 @@ describe MergeRequest do
112	let(:commit2) { mock('commit2', closes_issues: [issue1]) }	112	let(:commit2) { mock('commit2', closes_issues: [issue1]) }
113		113
114	before do	114	before do
115	- subject.stub(unmerged_commits: [commit0, commit1, commit2])	115	+ subject.stub(commits: [commit0, commit1, commit2])
116	end	116	end
117		117
118	it 'accesses the set of issues that will be closed on acceptance' do	118	it 'accesses the set of issues that will be closed on acceptance' do