diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index b343ba0b744c1ec274beb1a273af055c99e17ef6..dbbd2ad849e548c9cc5686e9cbcbcdb9063d550b 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/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 diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index dd65a9a8b86085c06fc87847cfd59bc5c56d1e68..58a24eb84cb4752e991ccc128056931e4a3982f6 100644 --- a/app/models/merge_request_diff.rb +++ b/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. diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 4a7e69308421399fb4e199edaf154e627700ae08..22596b4014ab3c77c62634139a2a10a0f3bd09fd 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/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 diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml index ac26aa569baba4c73a8f3f188913229786a28404..20c93930abc32f2ef040a280d3e7f5c3035714d9 100644 --- a/app/views/projects/merge_requests/widget/_open.html.haml +++ b/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 diff --git a/changelogs/unreleased/fix-merge-request-screen-deleted-source-branch.yml b/changelogs/unreleased/fix-merge-request-screen-deleted-source-branch.yml deleted file mode 100644 index a6bee989f6da79d91f7dc16c86dd75b059a47445..0000000000000000000000000000000000000000 --- a/changelogs/unreleased/fix-merge-request-screen-deleted-source-branch.yml +++ /dev/null @@ -1,4 +0,0 @@ ---- -title: Do not create a MergeRequestDiff record when source branch is deleted -merge_request: 7481 -author: diff --git a/changelogs/unreleased/hide-empty-merge-request-diffs.yml b/changelogs/unreleased/hide-empty-merge-request-diffs.yml new file mode 100644 index 0000000000000000000000000000000000000000..e32a51b555a7175d7721cdc2266cf4f9e2a16aee --- /dev/null +++ b/changelogs/unreleased/hide-empty-merge-request-diffs.yml @@ -0,0 +1,4 @@ +--- +title: Fix errors happening when source branch of merge request is removed and then restored +merge_request: 7568 +author: diff --git a/spec/features/merge_requests/deleted_source_branch_spec.rb b/spec/features/merge_requests/deleted_source_branch_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..778b3a90cf307d6a87a439dd0a17468c8aacea7b --- /dev/null +++ b/spec/features/merge_requests/deleted_source_branch_spec.rb @@ -0,0 +1,30 @@ +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 diff --git a/spec/features/merge_requests/merge_request_versions_spec.rb b/spec/features/merge_requests/merge_request_versions_spec.rb index 23cee891bace5aa726ab326c3f047c885cc6127c..09451f41de4241bb69182d865e8a147fc4c407ca 100644 --- a/spec/features/merge_requests/merge_request_versions_spec.rb +++ b/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 diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 0220f7e1db2a9a0289ed1756ef1b6f5b08b325db..e515bc9f89c2dabc066b3c9b776635a070026cca 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/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