Merge branch 'hide-empty-merge-request-diffs' into 'master'

Fix errors happening when source branch of merge request is removed and then restored

See merge request !7568

app/controllers/projects/merge_requests_controller.rb

@@ -82,12 +82,12 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     @merge_request_diff =
       if params[:diff_id]
-        @merge_request.merge_request_diffs.find(params[:diff_id])
+        @merge_request.merge_request_diffs.viewable.find(params[:diff_id])
       else
         @merge_request.merge_request_diff
       end
 
-    @merge_request_diffs = @merge_request.merge_request_diffs.select_without_diff
+    @merge_request_diffs = @merge_request.merge_request_diffs.viewable.select_without_diff
     @comparable_diffs = @merge_request_diffs.select { |diff| diff.id < @merge_request_diff.id }
 
     if params[:start_sha].present?
@@ -417,7 +417,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     response = {
       title: merge_request.title,
-      sha: merge_request.diff_head_commit.short_id,
+      sha: (merge_request.diff_head_commit.short_id if merge_request.diff_head_sha),
       status: status,
       coverage: coverage
     }
@@ -564,7 +564,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def define_pipelines_vars
     @pipelines = @merge_request.all_pipelines
 
-    if @pipelines.present?
+    if @pipelines.present? && @merge_request.commits.present?
       @pipeline = @pipelines.first
       @statuses = @pipeline.statuses.relevant
     end

app/models/merge_request_diff.rb

@@ -11,6 +11,9 @@ class MergeRequestDiff < ActiveRecord::Base
 
   belongs_to :merge_request
 
+  serialize :st_commits
+  serialize :st_diffs
+
   state_machine :state, initial: :empty do
     state :collected
     state :overflow
@@ -22,8 +25,7 @@ class MergeRequestDiff < ActiveRecord::Base
     state :overflow_diff_lines_limit
   end
 
-  serialize :st_commits
-  serialize :st_diffs
+  scope :viewable, -> { without_state(:empty) }
 
   # All diff information is collected from repository after object is created.
   # It allows you to override variables like head_commit_sha before getting diff.

app/services/merge_requests/refresh_service.rb

@@ -60,7 +60,15 @@ module MergeRequests
       merge_requests = filter_merge_requests(merge_requests)
 
       merge_requests.each do |merge_request|
-        reload_diff(merge_request) unless branch_removed?
+        if merge_request.source_branch == @branch_name || force_push?
+          merge_request.reload_diff
+        else
+          mr_commit_ids = merge_request.commits.map(&:id)
+          push_commit_ids = @commits.map(&:id)
+          matches = mr_commit_ids & push_commit_ids
+          merge_request.reload_diff if matches.any?
+        end
+
         merge_request.mark_as_unchecked
       end
     end
@@ -165,16 +173,5 @@ module MergeRequests
     def branch_removed?
       Gitlab::Git.blank_ref?(@newrev)
     end
-
-    def reload_diff(merge_request)
-      if merge_request.source_branch == @branch_name || force_push?
-        merge_request.reload_diff
-      else
-        mr_commit_ids = merge_request.commits.map(&:id)
-        push_commit_ids = @commits.map(&:id)
-        matches = mr_commit_ids & push_commit_ids
-        merge_request.reload_diff if matches.any?
-      end
-    end
   end
 end

app/views/projects/merge_requests/widget/_open.html.haml

@@ -9,10 +9,10 @@
 
     - if @project.archived?
       = render 'projects/merge_requests/widget/open/archived'
-    - elsif @merge_request.commits.blank?
-      = render 'projects/merge_requests/widget/open/nothing'
     - elsif @merge_request.branch_missing?
       = render 'projects/merge_requests/widget/open/missing_branch'
+    - elsif @merge_request.commits.blank?
+      = render 'projects/merge_requests/widget/open/nothing'
     - elsif @merge_request.unchecked?
       = render 'projects/merge_requests/widget/open/check'
     - elsif @merge_request.cannot_be_merged? && !resolved_conflicts

changelogs/unreleased/fix-merge-request-screen-deleted-source-branch.yml

----
-title: Do not create a MergeRequestDiff record when source branch is deleted
-merge_request: 7481
-author:

changelogs/unreleased/hide-empty-merge-request-diffs.yml

+---
+title: Fix errors happening when source branch of merge request is removed and then restored
+merge_request: 7568
+author:

spec/features/merge_requests/deleted_source_branch_spec.rb

+require 'spec_helper'
+
+describe 'Deleted source branch', feature: true, js: true do
+  let(:user) { create(:user) }
+  let(:merge_request) { create(:merge_request) }
+
+  before do
+    login_as user
+    merge_request.project.team << [user, :master]
+    merge_request.update!(source_branch: 'this-branch-does-not-exist')
+    visit namespace_project_merge_request_path(
+      merge_request.project.namespace,
+      merge_request.project, merge_request
+    )
+  end
+
+  it 'shows a message about missing source branch' do
+    expect(page).to have_content(
+      'Source branch this-branch-does-not-exist does not exist'
+    )
+  end
+
+  it 'hides Discussion, Commits and Changes tabs' do
+    within '.merge-request-details' do
+      expect(page).to have_no_content('Discussion')
+      expect(page).to have_no_content('Commits')
+      expect(page).to have_no_content('Changes')
+    end
+  end
+end

spec/features/merge_requests/merge_request_versions_spec.rb

@@ -3,11 +3,12 @@ require 'spec_helper'
 feature 'Merge Request versions', js: true, feature: true do
   let(:merge_request) { create(:merge_request, importing: true) }
   let(:project) { merge_request.source_project }
+  let!(:merge_request_diff1) { merge_request.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') }
+  let!(:merge_request_diff2) { merge_request.merge_request_diffs.create(head_commit_sha: nil) }
+  let!(:merge_request_diff3) { merge_request.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') }
 
   before do
     login_as :admin
-    merge_request.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9')
-    merge_request.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e')
     visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request)
   end
 
@@ -53,7 +54,7 @@ feature 'Merge Request versions', js: true, feature: true do
         project.namespace,
         project,
         merge_request.iid,
-        diff_id: 2,
+        diff_id: merge_request_diff3.id,
         start_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9'
       )
     end

spec/services/merge_requests/refresh_service_spec.rb

@@ -227,16 +227,6 @@ describe MergeRequests::RefreshService, services: true do
       end
     end
 
-    context 'when the source branch is deleted' do
-      it 'does not create a MergeRequestDiff record' do
-        refresh_service = service.new(@project, @user)
-
-        expect do
-          refresh_service.execute(@oldrev, Gitlab::Git::BLANK_SHA, 'refs/heads/master')
-        end.not_to change { MergeRequestDiff.count }
-      end
-    end
-
     def reload_mrs
       @merge_request.reload
       @fork_merge_request.reload