Merge branch 'id-optimize-mergeable-discussions-state' into 'master'

Optimize MergeRequest#mergeable_discussions_state? method

See merge request gitlab-org/gitlab!19988

app/models/concerns/noteable.rb

@@ -108,10 +108,6 @@ module Noteable
     discussions_resolvable? && resolvable_discussions.none?(&:to_be_resolved?)
   end
 
-  def discussions_to_be_resolved?
-    discussions_resolvable? && !discussions_resolved?
-  end
-
   def discussions_to_be_resolved
     @discussions_to_be_resolved ||= resolvable_discussions.select(&:to_be_resolved?)
   end

app/models/merge_request.rb

@@ -68,6 +68,7 @@ class MergeRequest < ApplicationRecord
   has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue
   has_many :pipelines_for_merge_request, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline'
   has_many :suggestions, through: :notes
+  has_many :unresolved_notes, -> { unresolved }, as: :noteable, class_name: 'Note'
 
   has_many :merge_request_assignees
   has_many :assignees, class_name: "User", through: :merge_request_assignees
@@ -211,7 +212,7 @@ class MergeRequest < ApplicationRecord
   scope :join_project, -> { joins(:target_project) }
   scope :references_project, -> { references(:target_project) }
   scope :with_api_entity_associations, -> {
-    preload(:assignees, :author, :notes, :labels, :milestone, :timelogs,
+    preload(:assignees, :author, :unresolved_notes, :labels, :milestone, :timelogs,
             latest_merge_request_diff: [:merge_request_diff_commits],
             metrics: [:latest_closed_by, :merged_by],
             target_project: [:route, { namespace: :route }],
@@ -923,7 +924,7 @@ class MergeRequest < ApplicationRecord
   def mergeable_discussions_state?
     return true unless project.only_allow_merge_if_all_discussions_are_resolved?
 
-    !discussions_to_be_resolved?
+    unresolved_notes.none?(&:to_be_resolved?)
   end
 
   def for_fork?

changelogs/unreleased/id-optimize-mergeable-discussions-state.yml

+---
+title: Optimize MergeRequest#mergeable_discussions_state? method
+merge_request: 19988
+author:
+type: performance

spec/lib/gitlab/import_export/all_models.yml

@@ -120,6 +120,7 @@ merge_requests:
 - pipelines_for_merge_request
 - merge_request_assignees
 - suggestions
+- unresolved_notes
 - assignees
 - reviews
 - approval_rules

spec/models/concerns/noteable_spec.rb

@@ -177,50 +177,6 @@ describe Noteable do
       end
     end
 
-    describe "#discussions_to_be_resolved?" do
-      context "when discussions are not resolvable" do
-        before do
-          allow(subject).to receive(:discussions_resolvable?).and_return(false)
-        end
-
-        it "returns false" do
-          expect(subject.discussions_to_be_resolved?).to be false
-        end
-      end
-
-      context "when discussions are resolvable" do
-        before do
-          allow(subject).to receive(:discussions_resolvable?).and_return(true)
-
-          allow(first_discussion).to receive(:resolvable?).and_return(true)
-          allow(second_discussion).to receive(:resolvable?).and_return(false)
-          allow(third_discussion).to receive(:resolvable?).and_return(true)
-        end
-
-        context "when all resolvable discussions are resolved" do
-          before do
-            allow(first_discussion).to receive(:resolved?).and_return(true)
-            allow(third_discussion).to receive(:resolved?).and_return(true)
-          end
-
-          it "returns false" do
-            expect(subject.discussions_to_be_resolved?).to be false
-          end
-        end
-
-        context "when some resolvable discussions are not resolved" do
-          before do
-            allow(first_discussion).to receive(:resolved?).and_return(true)
-            allow(third_discussion).to receive(:resolved?).and_return(false)
-          end
-
-          it "returns true" do
-            expect(subject.discussions_to_be_resolved?).to be true
-          end
-        end
-      end
-    end
-
     describe "#discussions_to_be_resolved" do
       before do
         allow(first_discussion).to receive(:to_be_resolved?).and_return(true)

spec/requests/api/merge_requests_spec.rb

@@ -701,16 +701,20 @@ describe API::MergeRequests do
       expect(json_response.first['id']).to eq merge_request_closed.id
     end
 
-    it 'avoids N+1 queries' do
-      control = ActiveRecord::QueryRecorder.new do
-        get api("/projects/#{project.id}/merge_requests", user)
-      end.count
+    context 'a project which enforces all discussions to be resolved' do
+      let!(:project) { create(:project, :repository, only_allow_merge_if_all_discussions_are_resolved: true) }
 
-      create(:merge_request, author: user, assignees: [user], source_project: project, target_project: project, created_at: base_time)
+      it 'avoids N+1 queries' do
+        control = ActiveRecord::QueryRecorder.new do
+          get api("/projects/#{project.id}/merge_requests", user)
+        end.count
 
-      expect do
-        get api("/projects/#{project.id}/merge_requests", user)
-      end.not_to exceed_query_limit(control)
+        create(:merge_request, author: user, assignees: [user], source_project: project, target_project: project, created_at: base_time)
+
+        expect do
+          get api("/projects/#{project.id}/merge_requests", user)
+        end.not_to exceed_query_limit(control)
+      end
     end
   end