Commit 2e250f36 authored by Igor's avatar Igor Committed by Sean McGivern

Optimize MergeRequest#mergeable_discussions_state? method

We don't need to build discussions to calculate this flag
It's sufficient to fetch the notes which are resolvable but
unresolved yet
parent 0bf5dd3e
...@@ -108,10 +108,6 @@ module Noteable ...@@ -108,10 +108,6 @@ module Noteable
discussions_resolvable? && resolvable_discussions.none?(&:to_be_resolved?) discussions_resolvable? && resolvable_discussions.none?(&:to_be_resolved?)
end end
def discussions_to_be_resolved?
discussions_resolvable? && !discussions_resolved?
end
def discussions_to_be_resolved def discussions_to_be_resolved
@discussions_to_be_resolved ||= resolvable_discussions.select(&:to_be_resolved?) @discussions_to_be_resolved ||= resolvable_discussions.select(&:to_be_resolved?)
end end
......
...@@ -68,6 +68,7 @@ class MergeRequest < ApplicationRecord ...@@ -68,6 +68,7 @@ class MergeRequest < ApplicationRecord
has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue 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 :pipelines_for_merge_request, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline'
has_many :suggestions, through: :notes has_many :suggestions, through: :notes
has_many :unresolved_notes, -> { unresolved }, as: :noteable, class_name: 'Note'
has_many :merge_request_assignees has_many :merge_request_assignees
has_many :assignees, class_name: "User", through: :merge_request_assignees has_many :assignees, class_name: "User", through: :merge_request_assignees
...@@ -211,7 +212,7 @@ class MergeRequest < ApplicationRecord ...@@ -211,7 +212,7 @@ class MergeRequest < ApplicationRecord
scope :join_project, -> { joins(:target_project) } scope :join_project, -> { joins(:target_project) }
scope :references_project, -> { references(:target_project) } scope :references_project, -> { references(:target_project) }
scope :with_api_entity_associations, -> { 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], latest_merge_request_diff: [:merge_request_diff_commits],
metrics: [:latest_closed_by, :merged_by], metrics: [:latest_closed_by, :merged_by],
target_project: [:route, { namespace: :route }], target_project: [:route, { namespace: :route }],
...@@ -923,7 +924,7 @@ class MergeRequest < ApplicationRecord ...@@ -923,7 +924,7 @@ class MergeRequest < ApplicationRecord
def mergeable_discussions_state? def mergeable_discussions_state?
return true unless project.only_allow_merge_if_all_discussions_are_resolved? return true unless project.only_allow_merge_if_all_discussions_are_resolved?
!discussions_to_be_resolved? unresolved_notes.none?(&:to_be_resolved?)
end end
def for_fork? def for_fork?
......
---
title: Optimize MergeRequest#mergeable_discussions_state? method
merge_request: 19988
author:
type: performance
...@@ -120,6 +120,7 @@ merge_requests: ...@@ -120,6 +120,7 @@ merge_requests:
- pipelines_for_merge_request - pipelines_for_merge_request
- merge_request_assignees - merge_request_assignees
- suggestions - suggestions
- unresolved_notes
- assignees - assignees
- reviews - reviews
- approval_rules - approval_rules
......
...@@ -177,50 +177,6 @@ describe Noteable do ...@@ -177,50 +177,6 @@ describe Noteable do
end end
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 describe "#discussions_to_be_resolved" do
before do before do
allow(first_discussion).to receive(:to_be_resolved?).and_return(true) allow(first_discussion).to receive(:to_be_resolved?).and_return(true)
......
...@@ -701,16 +701,20 @@ describe API::MergeRequests do ...@@ -701,16 +701,20 @@ describe API::MergeRequests do
expect(json_response.first['id']).to eq merge_request_closed.id expect(json_response.first['id']).to eq merge_request_closed.id
end end
it 'avoids N+1 queries' do context 'a project which enforces all discussions to be resolved' do
control = ActiveRecord::QueryRecorder.new do let!(:project) { create(:project, :repository, only_allow_merge_if_all_discussions_are_resolved: true) }
get api("/projects/#{project.id}/merge_requests", user)
end.count
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 create(:merge_request, author: user, assignees: [user], source_project: project, target_project: project, created_at: base_time)
get api("/projects/#{project.id}/merge_requests", user)
end.not_to exceed_query_limit(control) expect do
get api("/projects/#{project.id}/merge_requests", user)
end.not_to exceed_query_limit(control)
end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment