Merge branch 'new-resolvable-discussion' of gitlab.com:gitlab-org/gitlab-ce into new-resolvable-discussion

13 files changed, 73 insertions, 35 deletions

diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index b4dded7e27e..3d2258d5e3e 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -292,17 +292,6 @@ module Issuable
     self.class.to_ability_name
   end
 
-  # Convert this Issuable class name to a format usable by notifications.
-  #
-  # Examples:
-  #
-  #   issuable.class           # => MergeRequest
-  #   issuable.human_class_name # => "merge request"
-
-  def human_class_name
-    @human_class_name ||= self.class.name.titleize.downcase
-  end
-
   # Returns a Hash of attributes to be used for Twitter card metadata
   def card_attributes
     {
diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb
index 631df4757a4..772ff6a6d2f 100644
--- a/app/models/concerns/noteable.rb
+++ b/app/models/concerns/noteable.rb
@@ -1,4 +1,29 @@
 module Noteable
+  # Names of all implementers of `Noteable` that support resolvable notes.
+  RESOLVABLE_TYPES = %w(MergeRequest).freeze
+
+  def base_class_name
+    self.class.base_class.name
+  end
+
+  # Convert this Noteable class name to a format usable by notifications.
+  #
+  # Examples:
+  #
+  #   noteable.class           # => MergeRequest
+  #   noteable.human_class_name # => "merge request"
+  def human_class_name
+    @human_class_name ||= base_class_name.titleize.downcase
+  end
+
+  def supports_resolvable_notes?
+    RESOLVABLE_TYPES.include?(base_class_name)
+  end
+
+  def supports_discussions?
+    DiscussionNote::NOTEABLE_TYPES.include?(base_class_name)
+  end
+
   def discussion_notes
     notes
   end
diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb
index 5cb51196a94..dd979e7bb17 100644
--- a/app/models/concerns/resolvable_discussion.rb
+++ b/app/models/concerns/resolvable_discussion.rb
@@ -20,6 +20,8 @@ module ResolvableDiscussion
       :last_note
     )
 
+    delegate :potentially_resolvable?, to: :first_note
+
     delegate  :resolved_at,
               :resolved_by,
 
@@ -27,11 +29,6 @@ module ResolvableDiscussion
               allow_nil: true
   end
 
-  # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
-  def potentially_resolvable?
-    for_merge_request?
-  end
-
   def resolvable?
     return @resolvable if @resolvable.present?
 
diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb
index 2c70678429a..05eb6f86704 100644
--- a/app/models/concerns/resolvable_note.rb
+++ b/app/models/concerns/resolvable_note.rb
@@ -1,6 +1,7 @@
 module ResolvableNote
   extend ActiveSupport::Concern
 
+  # Names of all subclasses of `Note` that can be resolvable.
   RESOLVABLE_TYPES = %w(DiffNote DiscussionNote).freeze
 
   included do
@@ -8,10 +9,8 @@ module ResolvableNote
 
     validates :resolved_by, presence: true, if: :resolved?
 
-    # 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') }
+    # Keep this scope in sync with `#potentially_resolvable?`
+    scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: Noteable::RESOLVABLE_TYPES) }
     # Keep this scope in sync with `#resolvable?`
     scope :resolvable, -> { potentially_resolvable.user }
 
@@ -31,7 +30,10 @@ module ResolvableNote
     end
   end
 
-  delegate :potentially_resolvable?, to: :to_discussion
+  # Keep this method in sync with the `potentially_resolvable` scope
+  def potentially_resolvable?
+    RESOLVABLE_TYPES.include?(self.class.name) && noteable.supports_resolvable_notes?
+  end
 
   # Keep this method in sync with the `resolvable` scope
   def resolvable?
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 22912b30c6a..d9b7e484e0f 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -1,7 +1,13 @@
 # A discussion on merge request or commit diffs consisting of `DiffNote` notes.
+#
+# A discussion of this type can be resolvable.
 class DiffDiscussion < Discussion
   include DiscussionOnDiff
 
+  def self.note_class
+    DiffNote
+  end
+
   delegate  :position,
             :original_position,
 
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 9185a5a0ad8..1523244f8a8 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -1,4 +1,6 @@
 # A note on merge request or commit diffs
+#
+# A note of this type can be resolvable.
 class DiffNote < Note
   include NoteOnDiff
 
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index 8268a140403..2c0e6379e6a 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -1,4 +1,6 @@
 # A non-diff discussion on an issue, merge request, commit, or snippet, consisting of `DiscussionNote` notes.
+#
+# A discussion of this type can be resolvable.
 class Discussion
   include ResolvableDiscussion
 
@@ -54,6 +56,10 @@ class Discussion
     nil
   end
 
+  def self.note_class
+    DiscussionNote
+  end
+
   def initialize(notes, noteable = nil)
     @notes = notes
     @noteable = noteable
diff --git a/app/models/discussion_note.rb b/app/models/discussion_note.rb
index 337fbc8435c..e660b024083 100644
--- a/app/models/discussion_note.rb
+++ b/app/models/discussion_note.rb
@@ -1,7 +1,9 @@
-# A note in a non-diff discussion on an issue, merge request, commit, or snippet
+# A note in a non-diff discussion on an issue, merge request, commit, or snippet.
+#
+# A note of this type can be resolvable.
 class DiscussionNote < Note
+  # Names of all implementers of `Noteable` that support discussions.
   NOTEABLE_TYPES = %w(MergeRequest Issue Commit Snippet).freeze
-  RESOLVABLE_TYPES = %w(MergeRequest).freeze
 
   validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
 
diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb
index 42318fa3d6e..c3f21c55240 100644
--- a/app/models/individual_note_discussion.rb
+++ b/app/models/individual_note_discussion.rb
@@ -1,8 +1,10 @@
 # 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.
+#
+# A discussion of this type is never resolvable.
 class IndividualNoteDiscussion < Discussion
-  def potentially_resolvable?
-    false
+  def self.note_class
+    Note
   end
 
   def individual_note?
diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb
index 6515762c92d..cb2651a03f8 100644
--- a/app/models/legacy_diff_discussion.rb
+++ b/app/models/legacy_diff_discussion.rb
@@ -1,6 +1,9 @@
 # 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`.
+#
+# A discussion of this type is never resolvable.
 class LegacyDiffDiscussion < Discussion
   include DiscussionOnDiff
 
@@ -8,8 +11,8 @@ class LegacyDiffDiscussion < Discussion
     true
   end
 
-  def potentially_resolvable?
-    false
+  def self.note_class
+    LegacyDiffNote
   end
 
   def collapsed?
diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb
index b9fddb2ea79..9a77557ebcd 100644
--- a/app/models/legacy_diff_note.rb
+++ b/app/models/legacy_diff_note.rb
@@ -1,6 +1,9 @@
 # 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`.
+#
+# A note of this type is never resolvable.
 class LegacyDiffNote < Note
   include NoteOnDiff
 
diff --git a/app/models/note.rb b/app/models/note.rb
index cd4996bdbae..401e3d7bcbc 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -1,3 +1,6 @@
+# A note on the root of an issue, merge request, commit, or snippet.
+#
+# A note of this type is never resolvable.
 class Note < ActiveRecord::Base
   extend ActiveModel::Naming
   include Gitlab::CurrentSettings
@@ -225,11 +228,7 @@ class Note < ActiveRecord::Base
   end
 
   def can_be_discussion_note?
-    DiscussionNote::NOTEABLE_TYPES.include?(self.noteable_type) && !part_of_discussion?
-  end
-
-  def can_be_resolvable?
-    DiscussionNote::RESOLVABLE_TYPES.include?(self.noteable_type)
+    self.noteable.supports_discussions? && !part_of_discussion?
   end
 
   def discussion_class(noteable = nil)
diff --git a/app/models/out_of_context_discussion.rb b/app/models/out_of_context_discussion.rb
index 31c6d5cff8b..85794630f70 100644
--- a/app/models/out_of_context_discussion.rb
+++ b/app/models/out_of_context_discussion.rb
@@ -2,6 +2,8 @@
 # contains that commit, they are displayed as if they were a discussion.
 #
 # This represents one of those discussions, consisting of `Note` notes.
+#
+# A discussion of this type is never resolvable.
 class OutOfContextDiscussion < Discussion
   # Returns an array of discussion ID components
   def self.build_discussion_id(note)
@@ -13,8 +15,8 @@ class OutOfContextDiscussion < Discussion
   def self.override_discussion_id(note)
     discussion_id(note)
   end
-
-  def potentially_resolvable?
-    false
+  
+  def self.note_class
+    Note
   end
 end