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

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
             &nbsp;
             = 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 @@
   &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
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