From d4d522b683a9f31cc4e0715d943eae30118293d2 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 22 Nov 2016 13:27:32 +0000 Subject: Revert "Merge branch 'hide-empty-merge-request-diffs' into 'master'" This reverts merge request !7568 --- .../projects/merge_requests_controller.rb | 8 +++--- app/models/merge_request_diff.rb | 6 ++--- app/services/merge_requests/refresh_service.rb | 21 ++++++++------- .../projects/merge_requests/widget/_open.html.haml | 4 +-- ...-merge-request-screen-deleted-source-branch.yml | 4 +++ .../unreleased/hide-empty-merge-request-diffs.yml | 4 --- .../merge_requests/deleted_source_branch_spec.rb | 30 ---------------------- .../merge_requests/merge_request_versions_spec.rb | 7 +++-- .../merge_requests/refresh_service_spec.rb | 10 ++++++++ 9 files changed, 37 insertions(+), 57 deletions(-) create mode 100644 changelogs/unreleased/fix-merge-request-screen-deleted-source-branch.yml delete mode 100644 changelogs/unreleased/hide-empty-merge-request-diffs.yml delete mode 100644 spec/features/merge_requests/deleted_source_branch_spec.rb diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index dbbd2ad849e..b343ba0b744 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.viewable.find(params[:diff_id]) + @merge_request.merge_request_diffs.find(params[:diff_id]) else @merge_request.merge_request_diff end - @merge_request_diffs = @merge_request.merge_request_diffs.viewable.select_without_diff + @merge_request_diffs = @merge_request.merge_request_diffs.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 if merge_request.diff_head_sha), + sha: merge_request.diff_head_commit.short_id, 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? && @merge_request.commits.present? + if @pipelines.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 58a24eb84cb..dd65a9a8b86 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -11,9 +11,6 @@ class MergeRequestDiff < ActiveRecord::Base belongs_to :merge_request - serialize :st_commits - serialize :st_diffs - state_machine :state, initial: :empty do state :collected state :overflow @@ -25,7 +22,8 @@ class MergeRequestDiff < ActiveRecord::Base state :overflow_diff_lines_limit end - scope :viewable, -> { without_state(:empty) } + serialize :st_commits + serialize :st_diffs # 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 22596b4014a..4a7e6930842 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -60,15 +60,7 @@ module MergeRequests merge_requests = filter_merge_requests(merge_requests) merge_requests.each do |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 - + reload_diff(merge_request) unless branch_removed? merge_request.mark_as_unchecked end end @@ -173,5 +165,16 @@ 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 20c93930abc..ac26aa569ba 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.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.branch_missing? + = render 'projects/merge_requests/widget/open/missing_branch' - 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 new file mode 100644 index 00000000000..a6bee989f6d --- /dev/null +++ b/changelogs/unreleased/fix-merge-request-screen-deleted-source-branch.yml @@ -0,0 +1,4 @@ +--- +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 deleted file mode 100644 index e32a51b555a..00000000000 --- a/changelogs/unreleased/hide-empty-merge-request-diffs.yml +++ /dev/null @@ -1,4 +0,0 @@ ---- -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 deleted file mode 100644 index 778b3a90cf3..00000000000 --- a/spec/features/merge_requests/deleted_source_branch_spec.rb +++ /dev/null @@ -1,30 +0,0 @@ -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 09451f41de4..23cee891bac 100644 --- a/spec/features/merge_requests/merge_request_versions_spec.rb +++ b/spec/features/merge_requests/merge_request_versions_spec.rb @@ -3,12 +3,11 @@ 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 @@ -54,7 +53,7 @@ feature 'Merge Request versions', js: true, feature: true do project.namespace, project, merge_request.iid, - diff_id: merge_request_diff3.id, + diff_id: 2, 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 e515bc9f89c..0220f7e1db2 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -227,6 +227,16 @@ 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 -- cgit v1.2.1