Refactor resolvability checks based on type

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
     {

app/models/concerns/noteable.rb

 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

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?
 

app/models/concerns/resolvable_note.rb

 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?

app/models/diff_discussion.rb

 # 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,
 

app/models/diff_note.rb

 # A note on merge request or commit diffs
+#
+# A note of this type can be resolvable.
 class DiffNote < Note
   include NoteOnDiff
 

app/models/discussion.rb

 # 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

app/models/discussion_note.rb

-# 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 }
 

app/models/individual_note_discussion.rb

 # 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?

app/models/legacy_diff_discussion.rb

 # 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?

app/models/legacy_diff_note.rb

 # 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
 

app/models/note.rb

+# 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)

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)
@@ -14,7 +16,7 @@ class OutOfContextDiscussion < Discussion
     discussion_id(note)
   end
   
-  def potentially_resolvable?
-    false
+  def self.note_class
+    Note
   end
 end

app/services/issues/build_service.rb

@@ -36,12 +36,14 @@ module Issues
 
     def item_for_discussion(discussion)
       first_note_to_resolve = discussion.first_note_to_resolve || discussion.first_note
-      other_note_count = discussion.notes.size - 1
-      note_url = Gitlab::UrlBuilder.build(first_note_to_resolve)
 
       is_very_first_note = first_note_to_resolve == discussion.first_note
       action = is_very_first_note ? "started" : "commented on"
 
+      note_url = Gitlab::UrlBuilder.build(first_note_to_resolve)
+
+      other_note_count = discussion.notes.size - 1
+
       discussion_info = "- [ ] #{first_note_to_resolve.author.to_reference} #{action} a [discussion](#{note_url}): "
       discussion_info << " (+#{other_note_count} #{'comment'.pluralize(other_note_count)})" if other_note_count > 0
 

app/views/projects/notes/_comment_type_button.html.haml → app/views/projects/notes/_comment_button.html.haml

-- noteable_type = @note.noteable_type
+- noteable_name = @note.noteable.human_class_name
 
 .pull-left.btn-group.append-right-10.comment-type-dropdown.js-comment-type-dropdown
   %input.btn.btn-nr.btn-create.comment-btn.js-comment-button.js-comment-submit-button{ type: 'submit', value: 'Comment' }
+
   - if @note.can_be_discussion_note?
     = button_tag type: 'button', class: 'btn btn-nr dropdown-toggle comment-btn js-note-new-discussion js-disable-on-submit', data: { 'dropdown-trigger' => '#resolvable-comment-menu' }, 'aria-label' => 'Open comment type dropdown' do
       = icon('caret-down')
+
     %ul#resolvable-comment-menu.dropdown-menu{ data: { dropdown: true } }
-      %li#comment.droplab-item-selected{ data: { value: '', 'button-text' => 'Comment', 'secondary-button-text' => "Comment & close #{noteable_type.titleize.downcase}" } }
+      %li#comment.droplab-item-selected{ data: { value: '', 'button-text' => 'Comment', 'secondary-button-text' => "Comment & close #{noteable_name}" } }
         = icon('check')
         .description
           %strong Comment
-          %p= "Add a general comment to this #{noteable_type.titleize.downcase}."
-      %li.divider
-      - if @note.can_be_resolvable?
-        %li#discussion{ data: { value: 'DiscussionNote', 'button-text' => 'Start discussion', 'secondary-button-text' => "Start discussion & close #{noteable_type.titleize.downcase}" } }
-          = icon('check')
-          .description
-            %strong Start discussion
           %p
-              Discuss a specific suggestion or question that needs to be resolved.
-      - else
-        %li#discussion{ data: { value: 'DiscussionNote', 'button-text' => 'Start discussion', 'secondary-button-text' => "Start discussion & close #{noteable_type.titleize.downcase}"} }
+            Add a general comment to this #{noteable_name}.
+
+      %li.divider
+
+      %li#discussion{ data: { value: 'DiscussionNote', 'button-text' => 'Start discussion', 'secondary-button-text' => "Start discussion & close #{noteable_name}" } }
         = icon('check')
         .description
           %strong Start discussion
           %p
-              Discuss a specific suggestion or question.
+            = succeed '.' do
+              Discuss a specific suggestion or question
+              - if @note.noteable.supports_resolvable_notes?
+                that needs to be resolved

app/views/projects/notes/_form.html.haml

@@ -28,7 +28,7 @@
     .error-alert
 
   .note-form-actions.clearfix
-    = render partial: 'projects/notes/comment_type_button', locals: { show_close_button: true }
+    = render partial: 'projects/notes/comment_button'
 
     = yield(:note_actions)