diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index ee72c8ba72f..6988527a3be 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -334,9 +334,9 @@ class ProjectsController < Projects::ApplicationController :issues_tracker_id, :default_branch, :visibility_level, :import_url, :last_activity_at, :namespace_id, :avatar, :build_allow_git_fetch, :build_timeout_in_minutes, :build_coverage_regex, - :public_builds, :only_allow_merge_if_build_succeeds, + :public_builds, :only_allow_merge_if_build_succeeds, :request_access_enabled, :only_allow_merge_if_all_discussions_are_resolved, - :request_access_enabled, :lfs_enabled, project_feature_attributes + :lfs_enabled, project_feature_attributes ) end diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 79820e9a435..49127aecc63 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -298,28 +298,68 @@ describe Projects::MergeRequestsController do end end - context 'when project project has unresolved discussion' do - before do - project.update_column(:only_allow_merge_if_all_discussions_are_resolved, allowed) - end + describe 'only_allow_merge_if_all_discussions_are_resolved? setting' do + let(:merge_request) { create(:merge_request_with_diff_notes, source_project: project, author: user) } - context "when the only_allow_merge_if_all_discussions_are_resolved? is true" do - let(:allowed) { true } + context 'when enabled' do + before do + project.update_column(:only_allow_merge_if_all_discussions_are_resolved, true) + end - it 'returns :failed' do - merge_with_sha + context 'with unresolved discussion' do + before do + expect(merge_request).not_to be_discussions_resolved + end - expect(assigns(:status)).to eq(:failed) + it 'returns :failed' do + merge_with_sha + + expect(assigns(:status)).to eq(:failed) + end + end + + context 'with all discussions resolved' do + before do + merge_request.discussions.each { |d| d.resolve!(user) } + expect(merge_request).to be_discussions_resolved + end + + it 'returns :success' do + merge_with_sha + + expect(assigns(:status)).to eq(:success) + end end end - context "when the only_allow_merge_if_all_discussions_are_resolved? is false" do - let(:allowed) { false } + context 'when disabled' do + before do + project.update_column(:only_allow_merge_if_all_discussions_are_resolved, false) + end - it 'returns :failed' do - merge_with_sha + context 'with unresolved discussion' do + before do + expect(merge_request).not_to be_discussions_resolved + end - expect(assigns(:status)).to eq(:success) + it 'returns :success' do + merge_with_sha + + expect(assigns(:status)).to eq(:success) + end + end + + context 'with all discussions resolved' do + before do + merge_request.discussions.each { |d| d.resolve!(user) } + expect(merge_request).to be_discussions_resolved + end + + it 'returns :success' do + merge_with_sha + + expect(assigns(:status)).to eq(:success) + end end end end diff --git a/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb b/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb index 100ddda0167..7f11db3c417 100644 --- a/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb +++ b/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb @@ -1,30 +1,30 @@ require 'spec_helper' feature 'Check if mergeable with unresolved discussions', js: true, feature: true do - let!(:user) { create(:user) } - let!(:project) { create(:project, :public, only_allow_merge_if_all_discussions_are_resolved: allowed) } - let!(:merge_request) { create(:merge_request_with_diff_notes, source_project: project, author: user, title: "Bug NS-04" ) } + let(:user) { create(:user) } + let(:project) { create(:project) } + let!(:merge_request) { create(:merge_request_with_diff_notes, source_project: project, author: user) } before do login_as user project.team << [user, :master] end - context 'when only_allow_merge_if_all_discussions_are_resolved is false' do - let(:allowed) { false } - - it 'allows MR to be merged' do - visit_merge_request(merge_request) - - expect(page).to have_button 'Accept Merge Request' + context 'when project.only_allow_merge_if_all_discussions_are_resolved == true' do + before do + project.update_column(:only_allow_merge_if_all_discussions_are_resolved, true) end - end - context 'when only_allow_merge_if_all_discussions_are_resolved is true' do - let(:allowed) { true } + context 'with unresolved discussions' do + it 'does not allow to merge' do + visit_merge_request(merge_request) - context "when discussions are resolved" do + expect(page).not_to have_button 'Accept Merge Request' + expect(page).to have_content('This merge request has unresolved discussions') + end + end + context 'with all discussions resolved' do before do merge_request.discussions.each { |d| d.resolve!(user) } end @@ -35,14 +35,30 @@ feature 'Check if mergeable with unresolved discussions', js: true, feature: tru expect(page).to have_button 'Accept Merge Request' end end + end - context "when discussions are unresolved" do + context 'when project.only_allow_merge_if_all_discussions_are_resolved == false' do + before do + project.update_column(:only_allow_merge_if_all_discussions_are_resolved, false) + end + context 'with unresolved discussions' do it 'does not allow to merge' do visit_merge_request(merge_request) - expect(page).not_to have_button 'Accept Merge Request' - expect(page).to have_content('This merge request has unresolved discussions') + expect(page).to have_button 'Accept Merge Request' + end + end + + context 'with all discussions resolved' do + before do + merge_request.discussions.each { |d| d.resolve!(user) } + end + + it 'allows MR to be merged' do + visit_merge_request(merge_request) + + expect(page).to have_button 'Accept Merge Request' end end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index f3d0373e6d7..fb032a89d50 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -825,11 +825,8 @@ describe MergeRequest, models: true do end context 'when failed' do - before { allow(subject).to receive(:broken?) { false } } - - context 'when project settings restrict to merge only if build succeeds and build failed' do + context 'when #mergeable_ci_state? is false' do before do - project.only_allow_merge_if_build_succeeds = true allow(subject).to receive(:mergeable_ci_state?) { false } end @@ -838,9 +835,8 @@ describe MergeRequest, models: true do end end - context "when project settings restrict to merge only when all the discussions are resolved" do + context 'when #mergeable_discussions_state? is false' do before do - project.only_allow_merge_if_all_discussions_are_resolved = true allow(subject).to receive(:mergeable_discussions_state?) { false } end @@ -899,45 +895,42 @@ describe MergeRequest, models: true do end describe '#mergeable_discussions_state?' do - let!(:user) { create(:user) } - let!(:project) { create(:project, only_allow_merge_if_all_discussions_are_resolved: allowed) } + let(:merge_request) { create(:merge_request_with_diff_notes, source_project: project) } - subject { create(:merge_request_with_diff_notes, source_project: project) } + context 'when project.only_allow_merge_if_all_discussions_are_resolved == true' do + let(:project) { create(:project, only_allow_merge_if_all_discussions_are_resolved: true) } - context 'when is true' do - let(:allowed) { true } - - context 'when discussions are resolved' do + context 'with all discussions resolved' do before do - subject.discussions.each { |d| d.resolve!(user) } + merge_request.discussions.each { |d| d.resolve!(merge_request.author) } end it 'returns true' do - expect(subject.mergeable_discussions_state?).to be_truthy + expect(merge_request.mergeable_discussions_state?).to be_truthy end end - context 'when discussions are unresolved' do + context 'with unresolved discussions' do before do - subject.discussions.map(&:unresolve!) + merge_request.discussions.each(&:unresolve!) end it 'returns false' do - expect(subject.mergeable_discussions_state?).to be_falsey + expect(merge_request.mergeable_discussions_state?).to be_falsey end end end - context 'when is false' do - let(:allowed) { false } + context 'when project.only_allow_merge_if_all_discussions_are_resolved == false' do + let(:project) { create(:project, only_allow_merge_if_all_discussions_are_resolved: false) } - context 'when discussions are unresolved' do + context 'with unresolved discussions' do before do - subject.discussions.map(&:unresolve!) + merge_request.discussions.each(&:unresolve!) end it 'returns true' do - expect(subject.mergeable_discussions_state?).to be_truthy + expect(merge_request.mergeable_discussions_state?).to be_truthy end end end