Address review comments

12 files changed, 32 insertions, 14 deletions

diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb
index 3141c150ac9..22cd9bb7fd4 100644
--- a/app/models/concerns/resolvable_discussion.rb
+++ b/app/models/concerns/resolvable_discussion.rb
@@ -2,12 +2,14 @@ module ResolvableDiscussion
   extend ActiveSupport::Concern
 
   included do
-    memoized_values << :resolvable
-    memoized_values << :resolved
-    memoized_values << :first_note
-    memoized_values << :first_note_to_resolve
-    memoized_values << :last_resolved_note
-    memoized_values << :last_note
+    memoized_values.push(
+      :resolvable,
+      :resolved,
+      :first_note,
+      :first_note_to_resolve,
+      :last_resolved_note,
+      :last_note
+    )
 
     delegate  :resolved_at,
               :resolved_by,
diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb
index 2aba979938b..1155ec22369 100644
--- a/app/models/concerns/resolvable_note.rb
+++ b/app/models/concerns/resolvable_note.rb
@@ -8,7 +8,9 @@ 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 `Discussion` subclasses 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') }
     # Keep this scope in sync with `#resolvable?`
     scope :resolvable, -> { potentially_resolvable.user }
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 8acb70eb7cb..a89cce4bf5e 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -1,3 +1,4 @@
+# A discussion on merge request or commit diffs consisting of `DiffNote` notes
 class DiffDiscussion < Discussion
   include DiscussionOnDiff
 
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 6029fc42e9c..9185a5a0ad8 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -1,3 +1,4 @@
+# A note on merge request or commit diffs
 class DiffNote < Note
   include NoteOnDiff
 
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index 782db4044ed..a77076f02b6 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -39,6 +39,7 @@ class Discussion
     [: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]
   end
diff --git a/app/models/discussion_note.rb b/app/models/discussion_note.rb
index 510aefbf609..8957161805a 100644
--- a/app/models/discussion_note.rb
+++ b/app/models/discussion_note.rb
@@ -1,3 +1,4 @@
+# A note in a non-diff discussion on an issue, merge request, commit, or snippet
 class DiscussionNote < Note
   NOTEABLE_TYPES = %w(MergeRequest Issue Commit Snippet).freeze
 
diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb
index ebcf60beaf3..506b0a98528 100644
--- a/app/models/individual_note_discussion.rb
+++ b/app/models/individual_note_discussion.rb
@@ -1,3 +1,5 @@
+# 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
 class IndividualNoteDiscussion < Discussion
   # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
   def potentially_resolvable?
diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb
index 36612a28ba1..39b57f6d743 100644
--- a/app/models/legacy_diff_discussion.rb
+++ b/app/models/legacy_diff_discussion.rb
@@ -1,3 +1,4 @@
+# A discussion on merge request or commit diffs consisting of `LegacyDiffNote` notes
 class LegacyDiffDiscussion < Discussion
   include DiscussionOnDiff
 
diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb
index 0d86610e473..8fdebef042b 100644
--- a/app/models/legacy_diff_note.rb
+++ b/app/models/legacy_diff_note.rb
@@ -1,3 +1,4 @@
+# A note on merge request or commit diffs, using the legacy implementation
 class LegacyDiffNote < Note
   include NoteOnDiff
 
diff --git a/app/models/note.rb b/app/models/note.rb
index 9d4f99ac9c8..3d2decf6930 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -68,6 +68,7 @@ class Note < ActiveRecord::Base
   scope :user, ->{ where(system: false) }
   scope :common, ->{ where(noteable_type: ["", nil]) }
   scope :fresh, ->{ order(created_at: :asc, id: :asc) }
+  scope :updated_after, ->(time){ where('updated_at > ?', time) }
   scope :inc_author_project, ->{ includes(:project, :author) }
   scope :inc_author, ->{ includes(:author) }
   scope :inc_relations_for_view, -> do
@@ -238,18 +239,20 @@ class Note < ActiveRecord::Base
     discussion_class(noteable).override_discussion_id(self) || super()
   end
 
-  # Returns a discussion containing just this note
+  # Returns a discussion containing just this note.
+  # This method exists as an alternative to `#discussion` to use when the methods
+  # we intend to call on the Discussion object don't require it to have all of its notes,
+  # and just depend on the first note or the type of discussion. This saves us a DB query.
   def to_discussion(noteable = nil)
     Discussion.build([self], noteable)
   end
 
-  # Returns the entire discussion this note is part of
+  # Returns the entire discussion this note is part of.
+  # Consider using `#to_discussion` if we do not need to render the discussion
+  # and all its notes and if we don't care about the discussion's resolvability status.
   def discussion
-    if part_of_discussion?
-      self.noteable.notes.find_discussion(self.discussion_id) || to_discussion
-    else
-      to_discussion
-    end
+    full_discussion = self.noteable.notes.find_discussion(self.discussion_id) if part_of_discussion?
+    full_discussion || to_discussion
   end
 
   def part_of_discussion?
diff --git a/app/models/out_of_context_discussion.rb b/app/models/out_of_context_discussion.rb
index ecbfd97699f..7be9aa19707 100644
--- a/app/models/out_of_context_discussion.rb
+++ b/app/models/out_of_context_discussion.rb
@@ -1,3 +1,5 @@
+# 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.
 class OutOfContextDiscussion < Discussion
   # To make sure all out-of-context notes are displayed in one discussion,
   # we override the discussion ID to be a newly generated but consistent ID.
diff --git a/app/models/simple_discussion.rb b/app/models/simple_discussion.rb
index 41c679daf2d..cd2c142211c 100644
--- a/app/models/simple_discussion.rb
+++ b/app/models/simple_discussion.rb
@@ -1,2 +1,3 @@
+# A non-diff discussion on an issue, merge request, commit, or snippet, consisting of `DiscussionNote` notes
 class SimpleDiscussion < Discussion
 end