From ea155ccc3edd071a0ca5adfd774513892b19ab6f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= <alejorro70@gmail.com>
Date: Fri, 2 Sep 2016 17:29:47 -0300
Subject: [PATCH] Optimize discussion notes resolving and unresolving
Use `update_all` to only require one query per discussion to
update the notes resolved status. Some changes had to be made to
the discussion spec to accout for the fact that notes are not
individually updated now
---
CHANGELOG | 1 +
app/models/diff_note.rb | 18 +++++++
app/models/discussion.rb | 39 +++++++++-----
spec/factories/notes.rb | 5 ++
spec/models/diff_note_spec.rb | 37 +++++++++++++
spec/models/discussion_spec.rb | 96 +++++++++++++---------------------
6 files changed, 125 insertions(+), 71 deletions(-)
diff --git a/CHANGELOG b/CHANGELOG
index 982b4dd5e5a..880f85bf57a 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -94,6 +94,7 @@ v 8.11.5 (unreleased)
- Optimize branch lookups and force a repository reload for Repository#find_branch
- Fix member expiration date picker after update
- Fix suggested colors options for new labels in the admin area. !6138
+ - Optimize discussion notes resolving and unresolving
- Fix GitLab import button
- Remove gitorious from import_sources
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 4442cefc7e9..559b3075905 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -13,6 +13,11 @@ class DiffNote < Note
validate :positions_complete
validate :verify_supported
+ # Keep this scope in sync with the logic in `#resolvable?`
+ scope :resolvable, -> { user.where(noteable_type: 'MergeRequest') }
+ scope :resolved, -> { resolvable.where.not(resolved_at: nil) }
+ scope :unresolved, -> { resolvable.where(resolved_at: nil) }
+
after_initialize :ensure_original_discussion_id
before_validation :set_original_position, :update_position, on: :create
before_validation :set_line_code, :set_original_discussion_id
@@ -25,6 +30,16 @@ class DiffNote < Note
def build_discussion_id(noteable_type, noteable_id, position)
[super(noteable_type, noteable_id), *position.key].join("-")
end
+
+ # This method must be kept in sync with `#resolve!`
+ def resolve!(current_user)
+ unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id)
+ end
+
+ # This method must be kept in sync with `#unresolve!`
+ def unresolve!
+ resolved.update_all(resolved_at: nil, resolved_by_id: nil)
+ end
end
def new_diff_note?
@@ -73,6 +88,7 @@ class DiffNote < Note
self.position.diff_refs == diff_refs
end
+ # If you update this method remember to also update the scope `resolvable`
def resolvable?
!system? && for_merge_request?
end
@@ -83,6 +99,7 @@ class DiffNote < Note
self.resolved_at.present?
end
+ # If you update this method remember to also update `.resolve!`
def resolve!(current_user)
return unless resolvable?
return if resolved?
@@ -92,6 +109,7 @@ class DiffNote < Note
save!
end
+ # If you update this method remember to also update `.unresolve!`
def unresolve!
return unless resolvable?
return unless resolved?
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index 9676bc03470..de06c13481a 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -1,7 +1,7 @@
class Discussion
NUMBER_OF_TRUNCATED_DIFF_LINES = 16
- attr_reader :first_note, :last_note, :notes
+ attr_reader :notes
delegate :created_at,
:project,
@@ -36,8 +36,6 @@ class Discussion
end
def initialize(notes)
- @first_note = notes.first
- @last_note = notes.last
@notes = notes
end
@@ -70,17 +68,25 @@ class Discussion
end
def resolvable?
- return @resolvable if defined?(@resolvable)
+ return @resolvable if @resolvable.present?
@resolvable = diff_discussion? && notes.any?(&:resolvable?)
end
def resolved?
- return @resolved if defined?(@resolved)
+ return @resolved if @resolved.present?
@resolved = resolvable? && notes.none?(&:to_be_resolved?)
end
+ def first_note
+ @first_note ||= @notes.first
+ end
+
+ def last_note
+ @last_note ||= @notes.last
+ end
+
def resolved_notes
notes.select(&:resolved?)
end
@@ -100,17 +106,13 @@ class Discussion
def resolve!(current_user)
return unless resolvable?
- notes.each do |note|
- note.resolve!(current_user) if note.resolvable?
- end
+ update { |notes| notes.resolve!(current_user) }
end
def unresolve!
return unless resolvable?
- notes.each do |note|
- note.unresolve! if note.resolvable?
- end
+ update { |notes| notes.unresolve! }
end
def for_target?(target)
@@ -118,7 +120,7 @@ class Discussion
end
def active?
- return @active if defined?(@active)
+ return @active if @active.present?
@active = first_note.active?
end
@@ -174,4 +176,17 @@ class Discussion
prev_lines
end
+
+ private
+
+ def update
+ notes_relation = DiffNote.where(id: notes.map(&:id)).fresh
+ yield(notes_relation)
+
+ # Set the notes array to the updated notes
+ @notes = notes_relation.to_a
+
+ # Reset the memoized values
+ @last_resolved_note = @resolvable = @resolved = @first_note = @last_note = nil
+ end
end
diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index 83e38095feb..6919002dedc 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -28,6 +28,11 @@ FactoryGirl.define do
diff_refs: noteable.diff_refs
)
end
+
+ trait :resolved do
+ resolved_at { Time.now }
+ resolved_by { create(:user) }
+ end
end
factory :diff_note_on_commit, traits: [:on_commit], class: DiffNote do
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index 6a640474cfe..3db5937a4f3 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -31,6 +31,43 @@ describe DiffNote, models: true do
subject { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
+ describe ".resolve!" do
+ let(:current_user) { create(:user) }
+ let!(:commit_note) { create(:diff_note_on_commit) }
+ let!(:resolved_note) { create(:diff_note_on_merge_request, :resolved) }
+ let!(:unresolved_note) { create(:diff_note_on_merge_request) }
+
+ before do
+ described_class.resolve!(current_user)
+
+ commit_note.reload
+ resolved_note.reload
+ unresolved_note.reload
+ end
+
+ it 'resolves only the resolvable, not yet resolved notes' do
+ expect(commit_note.resolved_at).to be_nil
+ expect(resolved_note.resolved_by).not_to eq(current_user)
+ expect(unresolved_note.resolved_at).not_to be_nil
+ expect(unresolved_note.resolved_by).to eq(current_user)
+ end
+ end
+
+ describe ".unresolve!" do
+ let!(:resolved_note) { create(:diff_note_on_merge_request, :resolved) }
+
+ before do
+ described_class.unresolve!
+
+ resolved_note.reload
+ end
+
+ it 'unresolves the resolved notes' do
+ expect(resolved_note.resolved_by).to be_nil
+ expect(resolved_note.resolved_at).to be_nil
+ end
+ end
+
describe "#position=" do
context "when provided a string" do
it "sets the position" do
diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb
index 179f2e73662..0142706d140 100644
--- a/spec/models/discussion_spec.rb
+++ b/spec/models/discussion_spec.rb
@@ -238,27 +238,19 @@ describe Discussion, model: true do
context "when resolvable" do
let(:user) { create(:user) }
+ let(:second_note) { create(:diff_note_on_commit) } # unresolvable
before do
allow(subject).to receive(:resolvable?).and_return(true)
-
- allow(first_note).to receive(:resolvable?).and_return(true)
- allow(second_note).to receive(:resolvable?).and_return(false)
- allow(third_note).to receive(:resolvable?).and_return(true)
end
context "when all resolvable notes are resolved" do
before do
first_note.resolve!(user)
third_note.resolve!(user)
- end
- it "calls resolve! on every resolvable note" do
- expect(first_note).to receive(:resolve!).with(current_user)
- expect(second_note).not_to receive(:resolve!)
- expect(third_note).to receive(:resolve!).with(current_user)
-
- subject.resolve!(current_user)
+ first_note.reload
+ third_note.reload
end
it "doesn't change resolved_at on the resolved notes" do
@@ -309,46 +301,44 @@ describe Discussion, model: true do
first_note.resolve!(user)
end
- it "calls resolve! on every resolvable note" do
- expect(first_note).to receive(:resolve!).with(current_user)
- expect(second_note).not_to receive(:resolve!)
- expect(third_note).to receive(:resolve!).with(current_user)
-
- subject.resolve!(current_user)
- end
-
it "doesn't change resolved_at on the resolved note" do
expect(first_note.resolved_at).not_to be_nil
- expect { subject.resolve!(current_user) }.not_to change { first_note.resolved_at }
+ expect { subject.resolve!(current_user) }.
+ not_to change { first_note.reload.resolved_at }
end
it "doesn't change resolved_by on the resolved note" do
expect(first_note.resolved_by).to eq(user)
- expect { subject.resolve!(current_user) }.not_to change { first_note.resolved_by }
+ expect { subject.resolve!(current_user) }.
+ not_to change { first_note.reload && first_note.resolved_by }
end
it "doesn't change the resolved state on the resolved note" do
expect(first_note.resolved?).to be true
- expect { subject.resolve!(current_user) }.not_to change { first_note.resolved? }
+ expect { subject.resolve!(current_user) }.
+ not_to change { first_note.reload && first_note.resolved? }
end
it "sets resolved_at on the unresolved note" do
subject.resolve!(current_user)
+ third_note.reload
expect(third_note.resolved_at).not_to be_nil
end
it "sets resolved_by on the unresolved note" do
subject.resolve!(current_user)
+ third_note.reload
expect(third_note.resolved_by).to eq(current_user)
end
it "marks the unresolved note as resolved" do
subject.resolve!(current_user)
+ third_note.reload
expect(third_note.resolved?).to be true
end
@@ -373,16 +363,10 @@ describe Discussion, model: true do
end
context "when no resolvable notes are resolved" do
- it "calls resolve! on every resolvable note" do
- expect(first_note).to receive(:resolve!).with(current_user)
- expect(second_note).not_to receive(:resolve!)
- expect(third_note).to receive(:resolve!).with(current_user)
-
- subject.resolve!(current_user)
- end
-
it "sets resolved_at on the unresolved notes" do
subject.resolve!(current_user)
+ first_note.reload
+ third_note.reload
expect(first_note.resolved_at).not_to be_nil
expect(third_note.resolved_at).not_to be_nil
@@ -390,6 +374,8 @@ describe Discussion, model: true do
it "sets resolved_by on the unresolved notes" do
subject.resolve!(current_user)
+ first_note.reload
+ third_note.reload
expect(first_note.resolved_by).to eq(current_user)
expect(third_note.resolved_by).to eq(current_user)
@@ -397,6 +383,8 @@ describe Discussion, model: true do
it "marks the unresolved notes as resolved" do
subject.resolve!(current_user)
+ first_note.reload
+ third_note.reload
expect(first_note.resolved?).to be true
expect(third_note.resolved?).to be true
@@ -404,18 +392,24 @@ describe Discussion, model: true do
it "sets resolved_at" do
subject.resolve!(current_user)
+ first_note.reload
+ third_note.reload
expect(subject.resolved_at).not_to be_nil
end
it "sets resolved_by" do
subject.resolve!(current_user)
+ first_note.reload
+ third_note.reload
expect(subject.resolved_by).to eq(current_user)
end
it "marks as resolved" do
subject.resolve!(current_user)
+ first_note.reload
+ third_note.reload
expect(subject.resolved?).to be true
end
@@ -451,16 +445,10 @@ describe Discussion, model: true do
third_note.resolve!(user)
end
- it "calls unresolve! on every resolvable note" do
- expect(first_note).to receive(:unresolve!)
- expect(second_note).not_to receive(:unresolve!)
- expect(third_note).to receive(:unresolve!)
-
- subject.unresolve!
- end
-
it "unsets resolved_at on the resolved notes" do
subject.unresolve!
+ first_note.reload
+ third_note.reload
expect(first_note.resolved_at).to be_nil
expect(third_note.resolved_at).to be_nil
@@ -468,6 +456,8 @@ describe Discussion, model: true do
it "unsets resolved_by on the resolved notes" do
subject.unresolve!
+ first_note.reload
+ third_note.reload
expect(first_note.resolved_by).to be_nil
expect(third_note.resolved_by).to be_nil
@@ -475,6 +465,8 @@ describe Discussion, model: true do
it "unmarks the resolved notes as resolved" do
subject.unresolve!
+ first_note.reload
+ third_note.reload
expect(first_note.resolved?).to be false
expect(third_note.resolved?).to be false
@@ -482,12 +474,16 @@ describe Discussion, model: true do
it "unsets resolved_at" do
subject.unresolve!
+ first_note.reload
+ third_note.reload
expect(subject.resolved_at).to be_nil
end
it "unsets resolved_by" do
subject.unresolve!
+ first_note.reload
+ third_note.reload
expect(subject.resolved_by).to be_nil
end
@@ -504,40 +500,22 @@ describe Discussion, model: true do
first_note.resolve!(user)
end
- it "calls unresolve! on every resolvable note" do
- expect(first_note).to receive(:unresolve!)
- expect(second_note).not_to receive(:unresolve!)
- expect(third_note).to receive(:unresolve!)
-
- subject.unresolve!
- end
-
it "unsets resolved_at on the resolved note" do
subject.unresolve!
- expect(first_note.resolved_at).to be_nil
+ expect(subject.first_note.resolved_at).to be_nil
end
it "unsets resolved_by on the resolved note" do
subject.unresolve!
- expect(first_note.resolved_by).to be_nil
+ expect(subject.first_note.resolved_by).to be_nil
end
it "unmarks the resolved note as resolved" do
subject.unresolve!
- expect(first_note.resolved?).to be false
- end
- end
-
- context "when no resolvable notes are resolved" do
- it "calls unresolve! on every resolvable note" do
- expect(first_note).to receive(:unresolve!)
- expect(second_note).not_to receive(:unresolve!)
- expect(third_note).to receive(:unresolve!)
-
- subject.unresolve!
+ expect(subject.first_note.resolved?).to be false
end
end
end
--
GitLab