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

@@ -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

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?

app/models/discussion.rb

 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

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

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

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