From cc656a11992483911cefe035d579096581cfde57 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@selenight.nl>
Date: Thu, 6 Apr 2017 10:05:57 -0500
Subject: [PATCH] Refactor resolvability checks based on type

---
 app/models/concerns/issuable.rb               | 11 --------
 app/models/concerns/noteable.rb               | 25 +++++++++++++++++
 app/models/concerns/resolvable_discussion.rb  |  7 ++---
 app/models/concerns/resolvable_note.rb        | 12 ++++----
 app/models/diff_discussion.rb                 |  6 ++++
 app/models/diff_note.rb                       |  2 ++
 app/models/discussion.rb                      |  6 ++++
 app/models/discussion_note.rb                 |  6 ++--
 app/models/individual_note_discussion.rb      |  6 ++--
 app/models/legacy_diff_discussion.rb          |  7 +++--
 app/models/legacy_diff_note.rb                |  3 ++
 app/models/note.rb                            |  9 +++---
 app/models/out_of_context_discussion.rb       |  8 ++++--
 app/services/issues/build_service.rb          |  8 ++++--
 .../projects/notes/_comment_button.html.haml  | 28 +++++++++++++++++++
 .../notes/_comment_type_button.html.haml      | 28 -------------------
 app/views/projects/notes/_form.html.haml      |  2 +-
 17 files changed, 107 insertions(+), 67 deletions(-)
 create mode 100644 app/views/projects/notes/_comment_button.html.haml
 delete mode 100644 app/views/projects/notes/_comment_type_button.html.haml

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
diff --git a/app/services/issues/build_service.rb b/app/services/issues/build_service.rb
index 7b5858a8ccb..3a4f7b159f1 100644
--- a/app/services/issues/build_service.rb
+++ b/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
 
diff --git a/app/views/projects/notes/_comment_button.html.haml b/app/views/projects/notes/_comment_button.html.haml
new file mode 100644
index 00000000000..008363263b3
--- /dev/null
+++ b/app/views/projects/notes/_comment_button.html.haml
@@ -0,0 +1,28 @@
+- 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_name}" } }
+        = icon('check')
+        .description
+          %strong Comment
+          %p
+            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
+            = succeed '.' do
+              Discuss a specific suggestion or question
+              - if @note.noteable.supports_resolvable_notes?
+                that needs to be resolved
diff --git a/app/views/projects/notes/_comment_type_button.html.haml b/app/views/projects/notes/_comment_type_button.html.haml
deleted file mode 100644
index 1444a9d1b65..00000000000
--- a/app/views/projects/notes/_comment_type_button.html.haml
+++ /dev/null
@@ -1,28 +0,0 @@
-- noteable_type = @note.noteable_type
-
-.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}" } }
-        = 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}"} }
-          = icon('check')
-          .description
-            %strong Start discussion
-            %p
-              Discuss a specific suggestion or question.
diff --git a/app/views/projects/notes/_form.html.haml b/app/views/projects/notes/_form.html.haml
index 057c6801edf..0d835a9e949 100644
--- a/app/views/projects/notes/_form.html.haml
+++ b/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)