Address review comments

16 files changed, 97 insertions, 68 deletions

diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb
index 28abed967f0..87db0c810c3 100644
--- a/app/models/concerns/discussion_on_diff.rb
+++ b/app/models/concerns/discussion_on_diff.rb
@@ -1,3 +1,4 @@
+# Contains functionality shared between `DiffDiscussion` and `LegacyDiffDiscussion`.
 module DiscussionOnDiff
   extend ActiveSupport::Concern
 
diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb
index 9ce40f329d8..1a5a7007a2b 100644
--- a/app/models/concerns/note_on_diff.rb
+++ b/app/models/concerns/note_on_diff.rb
@@ -1,3 +1,4 @@
+# Contains functionality shared between `DiffNote` and `LegacyDiffNote`.
 module NoteOnDiff
   extend ActiveSupport::Concern
 
diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb
index d378152eb56..631df4757a4 100644
--- a/app/models/concerns/noteable.rb
+++ b/app/models/concerns/noteable.rb
@@ -12,6 +12,32 @@ module Noteable
   end
 
   def grouped_diff_discussions
+    # Doesn't use `discussion_notes`, because this may include commit diff notes
+    # besides MR diff notes, that we do no want to display on the MR Changes tab.
     notes.inc_relations_for_view.grouped_diff_discussions
   end
+
+  def resolvable_discussions
+    @resolvable_discussions ||= discussion_notes.resolvable.discussions(self)
+  end
+
+  def discussions_resolvable?
+    resolvable_discussions.any?(&:resolvable?)
+  end
+
+  def discussions_resolved?
+    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
+
+  def discussions_can_be_resolved_by?(user)
+    discussions_to_be_resolved.all? { |discussion| discussion.can_resolve?(user) }
+  end
 end
diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb
index 22cd9bb7fd4..5cb51196a94 100644
--- a/app/models/concerns/resolvable_discussion.rb
+++ b/app/models/concerns/resolvable_discussion.rb
@@ -2,6 +2,15 @@ module ResolvableDiscussion
   extend ActiveSupport::Concern
 
   included do
+    # A number of properties of this `Discussion`, like `first_note` and `resolvable?`, are memoized.
+    # When this discussion is resolved or unresolved, the values of these properties potentially change.
+    # To make sure all memoized values are reset when this happens, `update` resets all instance variables with names in
+    # `memoized_variables`. If you add a memoized method in `ResolvableDiscussion` or any `Discussion` subclass,
+    # please make sure the instance variable name is added to `memoized_values`, like below.
+    cattr_accessor :memoized_values, instance_accessor: false do
+      []
+    end
+
     memoized_values.push(
       :resolvable,
       :resolved,
@@ -78,4 +87,20 @@ module ResolvableDiscussion
 
     update { |notes| notes.unresolve! }
   end
+
+  private
+
+  def update
+    # Do not select `Note.resolvable`, so that system notes remain in the collection
+    notes_relation = Note.where(id: notes.map(&:id))
+
+    yield(notes_relation)
+
+    # Set the notes array to the updated notes
+    @notes = notes_relation.fresh.to_a
+
+    self.class.memoized_values.each do |var|
+      instance_variable_set(:"@#{var}", nil)
+    end
+  end
 end
diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb
index 1155ec22369..2c70678429a 100644
--- a/app/models/concerns/resolvable_note.rb
+++ b/app/models/concerns/resolvable_note.rb
@@ -8,7 +8,7 @@ module ResolvableNote
 
     validates :resolved_by, presence: true, if: :resolved?
 
-    # Keep this scope in sync with the logic in `#potentially_resolvable?` in `Discussion` subclasses that are resolvable.
+    # Keep this scope in sync with the logic in `#potentially_resolvable?` in subclasses of `Discussion` that are resolvable.
     # `RESOLVABLE_TYPES` should include names of all subclasses that are resolvable (where the method can return true), and
     # the scope should also match the criteria `ResolvableDiscussion#potentially_resolvable?` puts on resolvability.
     scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: 'MergeRequest') }
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index a89cce4bf5e..22912b30c6a 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -1,4 +1,4 @@
-# A discussion on merge request or commit diffs consisting of `DiffNote` notes
+# A discussion on merge request or commit diffs consisting of `DiffNote` notes.
 class DiffDiscussion < Discussion
   include DiscussionOnDiff
 
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index a77076f02b6..22130b8a508 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -1,8 +1,5 @@
+# A non-diff discussion on an issue, merge request, commit, or snippet, consisting of `DiscussionNote` notes.
 class Discussion
-  cattr_accessor :memoized_values, instance_accessor: false do
-    []
-  end
-
   include ResolvableDiscussion
 
   attr_reader :notes, :noteable
@@ -25,23 +22,34 @@ class Discussion
     notes.group_by { |n| n.discussion_id(noteable) }.values.map { |notes| build(notes, noteable) }
   end
 
+  # Returns an alphanumeric discussion ID based on `build_discussion_id`
   def self.discussion_id(note)
     Digest::SHA1.hexdigest(build_discussion_id(note).join("-"))
   end
 
-  # Optionally override the discussion ID at runtime depending on circumstances
-  def self.override_discussion_id(note)
-    nil
+  # Returns an array of discussion ID components
+  def self.build_discussion_id(note)
+    [*base_discussion_id(note), SecureRandom.hex]
   end
 
-  def self.build_discussion_id_base(note)
+  def self.base_discussion_id(note)
     noteable_id = note.noteable_id || note.commit_id
     [:discussion, note.noteable_type.try(:underscore), noteable_id]
   end
 
-  # Returns an array of discussion ID components
-  def self.build_discussion_id(note)
-    [*build_discussion_id_base(note), SecureRandom.hex]
+  # To turn a list of notes into a list of discussions, they are grouped by discussion ID.
+  # When notes on a commit are displayed in context of a merge request that contains that commit,
+  # these notes are to be displayed as if they were part of one discussion, even though they were actually
+  # individual notes on the commit with different discussion IDs, so that it's clear that these are not
+  # notes on the merge request itself.
+  # To get these out-of-context notes to end up in the same discussion, we need to get them to return the same
+  # `discussion_id` when this grouping happens. To enable this, `Note#discussion_id` calls out
+  # to the `override_discussion_id` method on the appropriate `Discussion` subclass, as determined by
+  # the `discussion_class` method on `Note` or a subclass of `Note`.
+  # If no override is necessary, return `nil`.
+  # For the case described above, see `OutOfContextDiscussion.override_discussion_id`.
+  def self.override_discussion_id(note)
+    nil
   end
 
   def initialize(notes, noteable = nil)
@@ -97,20 +105,4 @@ class Discussion
   def reply_attributes
     first_note.slice(:type, :noteable_type, :noteable_id, :commit_id, :discussion_id)
   end
-
-  private
-
-  def update
-    # Do not select `Note.resolvable`, so that system notes remain in the collection
-    notes_relation = Note.where(id: notes.map(&:id))
-
-    yield(notes_relation)
-
-    # Set the notes array to the updated notes
-    @notes = notes_relation.fresh.to_a
-
-    self.class.memoized_values.each do |var|
-      instance_variable_set(:"@#{var}", nil)
-    end
-  end
 end
diff --git a/app/models/discussion_note.rb b/app/models/discussion_note.rb
index 8957161805a..324d019cf79 100644
--- a/app/models/discussion_note.rb
+++ b/app/models/discussion_note.rb
@@ -5,6 +5,6 @@ class DiscussionNote < Note
   validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
 
   def discussion_class(*)
-    SimpleDiscussion
+    Discussion
   end
 end
diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb
index 506b0a98528..42318fa3d6e 100644
--- a/app/models/individual_note_discussion.rb
+++ b/app/models/individual_note_discussion.rb
@@ -1,7 +1,6 @@
 # A discussion to wrap a single `Note` note on the root of an issue, merge request,
-# commit, or snippet, that is not displayed as a discussion
+# commit, or snippet, that is not displayed as a discussion.
 class IndividualNoteDiscussion < Discussion
-  # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
   def potentially_resolvable?
     false
   end
diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb
index 39b57f6d743..6515762c92d 100644
--- a/app/models/legacy_diff_discussion.rb
+++ b/app/models/legacy_diff_discussion.rb
@@ -1,4 +1,6 @@
-# A discussion on merge request or commit diffs consisting of `LegacyDiffNote` notes
+# A discussion on merge request or commit diffs consisting of `LegacyDiffNote` notes.
+# All new diff discussions are of the type `DiffDiscussion`, but any diff discussions created
+# before the introduction of the new implementation still use `LegacyDiffDiscussion`.
 class LegacyDiffDiscussion < Discussion
   include DiscussionOnDiff
 
@@ -6,7 +8,6 @@ class LegacyDiffDiscussion < Discussion
     true
   end
 
-  # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
   def potentially_resolvable?
     false
   end
diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb
index 8fdebef042b..b9fddb2ea79 100644
--- a/app/models/legacy_diff_note.rb
+++ b/app/models/legacy_diff_note.rb
@@ -1,4 +1,6 @@
-# A note on merge request or commit diffs, using the legacy implementation
+# A note on merge request or commit diffs, using the legacy implementation.
+# All new diff notes are of the type `DiffNote`, but any diff notes created
+# before the introduction of the new implementation still use `LegacyDiffNote`.
 class LegacyDiffNote < Note
   include NoteOnDiff
 
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index a1efa17180e..77c902fdf09 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -478,30 +478,6 @@ class MergeRequest < ActiveRecord::Base
 
   alias_method :discussion_notes, :related_notes
 
-  def resolvable_discussions
-    @resolvable_discussions ||= notes.resolvable.discussions
-  end
-
-  def discussions_resolvable?
-    resolvable_discussions.any?(&:resolvable?)
-  end
-
-  def discussions_resolved?
-    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
-
-  def discussions_can_be_resolved_by?(user)
-    discussions_to_be_resolved.all? { |discussion| discussion.can_resolve?(user) }
-  end
-
   def mergeable_discussions_state?
     return true unless project.only_allow_merge_if_all_discussions_are_resolved?
 
diff --git a/app/models/note.rb b/app/models/note.rb
index 3d2decf6930..6cb9e84ce26 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -227,7 +227,8 @@ class Note < ActiveRecord::Base
 
   def discussion_class(noteable = nil)
     # When commit notes are rendered on an MR's Discussion page, they are
-    # displayed in one discussion instead of individually
+    # displayed in one discussion instead of individually.
+    # See also `#discussion_id` and `Discussion.override_discussion_id`.
     if noteable && noteable != self.noteable
       OutOfContextDiscussion
     else
@@ -235,6 +236,7 @@ class Note < ActiveRecord::Base
     end
   end
 
+  # See `Discussion.override_discussion_id` for details.
   def discussion_id(noteable = nil)
     discussion_class(noteable).override_discussion_id(self) || super()
   end
diff --git a/app/models/out_of_context_discussion.rb b/app/models/out_of_context_discussion.rb
index 7be9aa19707..62b62ea726c 100644
--- a/app/models/out_of_context_discussion.rb
+++ b/app/models/out_of_context_discussion.rb
@@ -1,13 +1,18 @@
-# A discussion to wrap a number of `Note` notes on the root of a commit when they
-# are displayed in context of a merge request as if they were part of a discussion.
+# When notes on a commit are displayed in the context of a merge request that contains that commit,
+# they are displayed as if they were a discussion.
+# This represents one of those discussions, consisting of `Note` notes.
 class OutOfContextDiscussion < Discussion
-  # To make sure all out-of-context notes are displayed in one discussion,
+  # Returns an array of discussion ID components
+  def self.build_discussion_id(note)
+    base_discussion_id(note)
+  end
+
+  # To make sure all out-of-context notes end up grouped as one discussion,
   # we override the discussion ID to be a newly generated but consistent ID.
   def self.override_discussion_id(note)
-    Digest::SHA1.hexdigest(build_discussion_id_base(note).join("-"))
+    discussion_id(note)
   end
 
-  # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
   def potentially_resolvable?
     false
   end
diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb
index 7d65b2b7993..bfaf0eb2fae 100644
--- a/app/models/sent_notification.rb
+++ b/app/models/sent_notification.rb
@@ -102,6 +102,8 @@ class SentNotification < ActiveRecord::Base
     if self.in_reply_to_discussion_id.present?
       attrs[:in_reply_to_discussion_id] = self.in_reply_to_discussion_id
     else
+      # Remove in GitLab 10.0, when we will not support replying to SentNotifications
+      # that don't have `in_reply_to_discussion_id` anymore.
       attrs.merge!(
         type: self.note_type,
 
diff --git a/app/models/simple_discussion.rb b/app/models/simple_discussion.rb
deleted file mode 100644
index cd2c142211c..00000000000
--- a/app/models/simple_discussion.rb
+++ /dev/null
@@ -1,3 +0,0 @@
-# A non-diff discussion on an issue, merge request, commit, or snippet, consisting of `DiscussionNote` notes
-class SimpleDiscussion < Discussion
-end