Complete and improve specs

Signed-off-by: Rémy Coutable <remy@rymai.me>

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
 

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

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

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