Add specs

17 files changed, 104 insertions, 78 deletions

diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index 186098c67b7..8f82021a464 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/app/controllers/projects/notes_controller.rb
@@ -1,4 +1,5 @@
 class Projects::NotesController < Projects::ApplicationController
+  include NotesHelper
   include ToggleAwardEmoji
 
   # Authorize
@@ -12,7 +13,8 @@ class Projects::NotesController < Projects::ApplicationController
 
     notes_json = { notes: [], last_fetched_at: current_fetched_at }
 
-    @notes = notes_finder.execute.inc_author
+    @notes = notes_finder.execute.inc_relations_for_view
+    @notes = prepare_notes_for_rendering(@notes)
 
     @notes.each do |note|
       next if note.cross_reference_not_visible_for?(current_user)
@@ -117,7 +119,7 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def discussion_html(discussion)
-    return if discussion.render_as_individual_notes?
+    return if discussion.individual_note?
 
     render_to_string(
       "discussions/_discussion",
@@ -153,21 +155,20 @@ class Projects::NotesController < Projects::ApplicationController
 
   def note_json(note)
     attrs = {
-      id: note.id
+      commands_changes: note.commands_changes
     }
 
     if note.persisted?
-      Banzai::NoteRenderer.render([note], @project, current_user)
-
       attrs.merge!(
         valid: true,
+        id: note.id,
         discussion_id: note.discussion_id(noteable),
         html: note_html(note),
         note: note.note
       )
 
       discussion = note.to_discussion(noteable)
-      unless discussion.render_as_individual_notes?
+      unless discussion.individual_note?
         attrs.merge!(
           discussion_resolvable: discussion.resolvable?,
 
@@ -187,7 +188,6 @@ class Projects::NotesController < Projects::ApplicationController
       )
     end
 
-    attrs[:commands_changes] = note.commands_changes
     attrs
   end
 
diff --git a/app/models/commit_discussion.rb b/app/models/commit_discussion.rb
deleted file mode 100644
index bcca3155335..00000000000
--- a/app/models/commit_discussion.rb
+++ /dev/null
@@ -1,9 +0,0 @@
-class CommitDiscussion < Discussion
-  def self.override_discussion_id(note)
-    discussion_id(note)
-  end
-
-  def potentially_resolvable?
-    false
-  end
-end
diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb
index eecb77ebf80..2aba979938b 100644
--- a/app/models/concerns/resolvable_note.rb
+++ b/app/models/concerns/resolvable_note.rb
@@ -1,13 +1,18 @@
 module ResolvableNote
   extend ActiveSupport::Concern
 
+  RESOLVABLE_TYPES = %w(DiffNote DiscussionNote).freeze
+
   included do
     belongs_to :resolved_by, class_name: "User"
 
     validates :resolved_by, presence: true, if: :resolved?
 
-    # Keep this scope in sync with the logic in `#resolvable?` in `Note` subclasses that are resolvable
-    scope :resolvable, -> { where(type: %w(DiffNote DiscussionNote)).user.where(noteable_type: 'MergeRequest') }
+    # Keep this scope in sync with the logic in `#potentially_resolvable?` in `Discussion` subclasses that are resolvable
+    scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: 'MergeRequest') }
+    # Keep this scope in sync with `#resolvable?`
+    scope :resolvable, -> { potentially_resolvable.user }
+
     scope :resolved, -> { resolvable.where.not(resolved_at: nil) }
     scope :unresolved, -> { resolvable.where(resolved_at: nil) }
   end
@@ -24,9 +29,11 @@ module ResolvableNote
     end
   end
 
-  # If you update this method remember to also update the scope `resolvable`
+  delegate :potentially_resolvable?, to: :to_discussion
+
+  # Keep this method in sync with the `resolvable` scope
   def resolvable?
-    to_discussion.potentially_resolvable? && !system?
+    potentially_resolvable? && !system?
   end
 
   def resolved?
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index e5f437f8647..d24491b44e7 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -1,6 +1,8 @@
 class DiffNote < Note
   include NoteOnDiff
 
+  NOTEABLE_TYPES = %w(MergeRequest Commit).freeze
+
   serialize :original_position, Gitlab::Diff::Position
   serialize :position, Gitlab::Diff::Position
 
@@ -8,7 +10,7 @@ class DiffNote < Note
   validates :position, presence: true
   validates :diff_line, presence: true
   validates :line_code, presence: true, line_code: true
-  validates :noteable_type, inclusion: { in: %w(Commit MergeRequest) }
+  validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
   validate :positions_complete
   validate :verify_supported
 
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index 8ab9031e42c..6e97a4862ed 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -1,7 +1,7 @@
 class Discussion
   MEMOIZED_VALUES = [] # rubocop:disable Style/MutableConstant
 
-  attr_reader :notes
+  attr_reader :notes, :noteable
 
   delegate  :created_at,
             :project,
@@ -61,6 +61,13 @@ class Discussion
     @noteable = noteable
   end
 
+  def ==(other)
+    other.class == self.class &&
+      other.noteable == self.noteable &&
+      other.id == self.id &&
+      other.notes == self.notes
+  end
+
   def last_updated_at
     last_note.created_at
   end
@@ -83,7 +90,7 @@ class Discussion
     false
   end
 
-  def render_as_individual_notes?
+  def individual_note?
     false
   end
 
@@ -91,8 +98,9 @@ class Discussion
     notes.length == 1
   end
 
+  # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
   def potentially_resolvable?
-    first_note.for_merge_request?
+    for_merge_request?
   end
 
   def resolvable?
@@ -162,12 +170,7 @@ class Discussion
   end
 
   def collapsed?
-    if resolvable?
-      # New diff discussions only disappear once they are marked resolved
-      resolved?
-    else
-      false
-    end
+    resolved?
   end
 
   def expanded?
diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb
index 8c82d217126..585b8527883 100644
--- a/app/models/individual_note_discussion.rb
+++ b/app/models/individual_note_discussion.rb
@@ -3,11 +3,12 @@ class IndividualNoteDiscussion < Discussion
     [*super(note), SecureRandom.hex]
   end
 
+  # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
   def potentially_resolvable?
     false
   end
 
-  def render_as_individual_notes?
+  def individual_note?
     true
   end
 end
diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb
index e13f17abdbe..eb9766a9ffe 100644
--- a/app/models/legacy_diff_discussion.rb
+++ b/app/models/legacy_diff_discussion.rb
@@ -11,6 +11,7 @@ class LegacyDiffDiscussion < DiffDiscussion
     true
   end
 
+  # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
   def potentially_resolvable?
     false
   end
diff --git a/app/models/note.rb b/app/models/note.rb
index 06ceb60b982..00a58afd2b6 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -56,7 +56,7 @@ class Note < ActiveRecord::Base
 
   validate unless: [:for_commit?, :importing?, :for_personal_snippet?] do |note|
     unless note.noteable.try(:project) == note.project
-      errors.add(:invalid_project, 'Note and noteable project mismatch')
+      errors.add(:project, 'does not match noteable project')
     end
   end
 
@@ -236,14 +236,14 @@ class Note < ActiveRecord::Base
   end
 
   def can_be_discussion_note?
-    DiscussionNote::NOTEABLE_TYPES.include?(self.noteable_type)
+    DiscussionNote::NOTEABLE_TYPES.include?(self.noteable_type) && !part_of_discussion?
   end
 
   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
-    if noteable && noteable != self.noteable && for_commit?
-      CommitDiscussion
+    if noteable && noteable != self.noteable
+      OutOfContextDiscussion
     else
       IndividualNoteDiscussion
     end
@@ -268,7 +268,24 @@ class Note < ActiveRecord::Base
   end
 
   def part_of_discussion?
-    !to_discussion.render_as_individual_notes?
+    !to_discussion.individual_note?
+  end
+
+  def in_reply_to?(other)
+    case other
+    when Note
+      if part_of_discussion?
+        in_reply_to?(other.noteable) && in_reply_to?(other.to_discussion)
+      else
+        in_reply_to?(other.noteable)
+      end
+    when Discussion
+      self.discussion_id == other.id
+    when Noteable
+      self.noteable == other
+    else
+      false
+    end
   end
 
   private
diff --git a/app/models/out_of_context_discussion.rb b/app/models/out_of_context_discussion.rb
new file mode 100644
index 00000000000..0019064e25c
--- /dev/null
+++ b/app/models/out_of_context_discussion.rb
@@ -0,0 +1,12 @@
+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.
+  def self.override_discussion_id(note)
+    discussion_id(note)
+  end
+
+  # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
+  def potentially_resolvable?
+    false
+  end
+end
diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb
index 6770af6fffd..81fc2ddac77 100644
--- a/app/models/sent_notification.rb
+++ b/app/models/sent_notification.rb
@@ -23,9 +23,7 @@ class SentNotification < ActiveRecord::Base
       find_by(reply_key: reply_key)
     end
 
-    def record(noteable, recipient_id, reply_key, attrs = {})
-      return unless reply_key
-
+    def record(noteable, recipient_id, reply_key = self.reply_key, attrs = {})
       noteable_id = nil
       commit_id = nil
       if noteable.is_a?(Commit)
@@ -47,7 +45,7 @@ class SentNotification < ActiveRecord::Base
       create(attrs)
     end
 
-    def record_note(note, recipient_id, reply_key, attrs = {})
+    def record_note(note, recipient_id, reply_key = self.reply_key, attrs = {})
       attrs[:in_reply_to_discussion_id] = note.original_discussion_id
 
       record(note.noteable, recipient_id, reply_key, attrs)
@@ -87,7 +85,14 @@ class SentNotification < ActiveRecord::Base
     self.reply_key
   end
 
-  def note_params
+  def create_reply(message, dryrun: false)
+    klass = dryrun ? Notes::BuildService : Notes::CreateService
+    klass.new(self.project, self.recipient, reply_params.merge(note: message)).execute
+  end
+
+  private
+
+  def reply_params
     attrs = {
       noteable_type: self.noteable_type,
       noteable_id: self.noteable_id,
@@ -111,10 +116,12 @@ class SentNotification < ActiveRecord::Base
     attrs
   end
 
-  private
-
   def note_valid
-    Notes::BuildService.new(self.project, self.recipient, note_params.merge(note: 'Test')).execute.valid?
+    note = create_reply('Test', dryrun: true)
+
+    unless note.valid?
+      self.errors.add(:base, "Note parameters are invalid: #{note.errors.full_messages.to_sentence}")
+    end
   end
 
   def keep_around_commit
diff --git a/app/services/concerns/issues/resolve_discussions.rb b/app/services/concerns/issues/resolve_discussions.rb
index 378967f0231..910a2a15e5d 100644
--- a/app/services/concerns/issues/resolve_discussions.rb
+++ b/app/services/concerns/issues/resolve_discussions.rb
@@ -21,7 +21,7 @@ module Issues
       @discussions_to_resolve ||=
         if discussion_to_resolve_id
           discussion_or_nil = merge_request_to_resolve_discussions_of
-                                .find_diff_discussion(discussion_to_resolve_id)
+                                .find_discussion(discussion_to_resolve_id)
           Array(discussion_or_nil)
         else
           merge_request_to_resolve_discussions_of
diff --git a/app/services/notes/build_service.rb b/app/services/notes/build_service.rb
index 47ed3e3fc3d..8d5a022d319 100644
--- a/app/services/notes/build_service.rb
+++ b/app/services/notes/build_service.rb
@@ -1,9 +1,6 @@
 module Notes
   class BuildService < BaseService
     def execute
-      # TODO: Remove when we use a selectbox instead of a submit button
-      params[:type] = DiscussionNote.name if params.delete(:new_discussion)
-
       in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id)
       if project && in_reply_to_discussion_id.present?
         discussion =
@@ -17,6 +14,9 @@ module Notes
         end
 
         params.merge!(discussion.reply_attributes)
+      elsif params.delete(:new_discussion)
+        # TODO: Remove when we use a selectbox instead of a submit button
+        params[:type] = DiscussionNote.name
       end
 
       note = Note.new(params)
diff --git a/app/views/notify/new_issue_email.html.haml b/app/views/notify/new_issue_email.html.haml
index d1855568215..c762578971a 100644
--- a/app/views/notify/new_issue_email.html.haml
+++ b/app/views/notify/new_issue_email.html.haml
@@ -1,9 +1,11 @@
 - if current_application_settings.email_author_in_body
-  %div
-    #{link_to @issue.author_name, user_url(@issue.author)} wrote:
-- if @issue.description
-  = markdown(@issue.description, pipeline: :email, author: @issue.author)
+  %p.details
+    #{link_to @issue.author_name, user_url(@issue.author)} created an issue:
 
 - if @issue.assignee_id.present?
   %p
     Assignee: #{@issue.assignee_name}
+
+- if @issue.description
+  %div
+  = markdown(@issue.description, pipeline: :email, author: @issue.author)
diff --git a/app/views/notify/new_mention_in_issue_email.html.haml b/app/views/notify/new_mention_in_issue_email.html.haml
index 02f21baa368..d7c22219285 100644
--- a/app/views/notify/new_mention_in_issue_email.html.haml
+++ b/app/views/notify/new_mention_in_issue_email.html.haml
@@ -1,12 +1,4 @@
 %p
   You have been mentioned in an issue.
 
-- if current_application_settings.email_author_in_body
-  %div
-    #{link_to @issue.author_name, user_url(@issue.author)} wrote:
-- if @issue.description
-  = markdown(@issue.description, pipeline: :email, author: @issue.author)
-
-- if @issue.assignee_id.present?
-  %p
-    Assignee: #{@issue.assignee_name}
+= render template: 'new_issue_email'
diff --git a/app/views/notify/new_mention_in_merge_request_email.html.haml b/app/views/notify/new_mention_in_merge_request_email.html.haml
index cbd434be02a..07d8c301988 100644
--- a/app/views/notify/new_mention_in_merge_request_email.html.haml
+++ b/app/views/notify/new_mention_in_merge_request_email.html.haml
@@ -1,15 +1,4 @@
 %p
   You have been mentioned in Merge Request #{@merge_request.to_reference}
 
-- if current_application_settings.email_author_in_body
-  %div
-    #{link_to @merge_request.author_name, user_url(@merge_request.author)} wrote:
-%p.details
-  != merge_path_description(@merge_request, '&rarr;')
-
-- if @merge_request.assignee_id.present?
-  %p
-    Assignee: #{@merge_request.author_name} &rarr; #{@merge_request.assignee_name}
-
-- if @merge_request.description
-  = markdown(@merge_request.description, pipeline: :email, author: @merge_request.author)
+= render template: 'new_merge_request_email'
diff --git a/app/views/notify/new_merge_request_email.html.haml b/app/views/notify/new_merge_request_email.html.haml
index 8890b300f7d..951c96bdb9c 100644
--- a/app/views/notify/new_merge_request_email.html.haml
+++ b/app/views/notify/new_merge_request_email.html.haml
@@ -1,12 +1,14 @@
 - if current_application_settings.email_author_in_body
-  %div
-    #{link_to @merge_request.author_name, user_url(@merge_request.author)} wrote:
+  %p.details
+    #{link_to @merge_request.author_name, user_url(@merge_request.author)} created a merge request:
+
 %p.details
   != merge_path_description(@merge_request, '&rarr;')
 
 - if @merge_request.assignee_id.present?
   %p
-    Assignee: #{@merge_request.author_name} &rarr; #{@merge_request.assignee_name}
+    Assignee: #{@merge_request.assignee_name}
 
 - if @merge_request.description
-  = markdown(@merge_request.description, pipeline: :email, author: @merge_request.author)
+  %div
+    = markdown(@merge_request.description, pipeline: :email, author: @merge_request.author)
diff --git a/app/views/projects/notes/_notes.html.haml b/app/views/projects/notes/_notes.html.haml
index d619f7ac262..2b2bab09c74 100644
--- a/app/views/projects/notes/_notes.html.haml
+++ b/app/views/projects/notes/_notes.html.haml
@@ -1,6 +1,6 @@
 - if defined?(@discussions)
   - @discussions.each do |discussion|
-    - if discussion.render_as_individual_notes?
+    - if discussion.individual_note?
       = render partial: "projects/notes/note", collection: discussion.notes, as: :note
     - else
       = render 'discussions/discussion', discussion: discussion