Optimize discussion notes resolving and unresolving21109-discussion-resolve-runs-a-single-update-query-per-note-but-should-run-a-single-update-query-for-all-notes-instead

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

2 files changed, 45 insertions, 12 deletions

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