Add specs

app/controllers/projects/notes_controller.rb

 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

app/models/commit_discussion.rb

-class CommitDiscussion < Discussion
-  def self.override_discussion_id(note)
-    discussion_id(note)
-  end
-
-  def potentially_resolvable?
-    false
-  end
-end

app/models/concerns/resolvable_note.rb

 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?

app/models/diff_note.rb

 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

app/models/discussion.rb

 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?

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

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

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

app/models/out_of_context_discussion.rb

+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

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

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

app/services/notes/build_service.rb

 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)

app/views/notify/new_issue_email.html.haml

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

app/views/notify/new_mention_in_issue_email.html.haml

 %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'

app/views/notify/new_mention_in_merge_request_email.html.haml

 %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, '→')
-
-- if @merge_request.assignee_id.present?
-  %p
-    Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name}
-
-- if @merge_request.description
-  = markdown(@merge_request.description, pipeline: :email, author: @merge_request.author)
+= render template: 'new_merge_request_email'

app/views/notify/new_merge_request_email.html.haml

 - 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, '→')
 - if @merge_request.assignee_id.present?
   %p
-    Assignee: #{@merge_request.author_name} → #{@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)

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

 - 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

lib/gitlab/email/handler/create_note_handler.rb

-
 require 'gitlab/email/handler/base_handler'
 require 'gitlab/email/handler/reply_processing'
@@ -42,11 +41,7 @@ module Gitlab
         end
         def create_note
-          Notes::CreateService.new(
-            project,
-            author,
-            sent_notification.note_params.merge(note: message)
-          ).execute
+          sent_notification.create_reply(message)
         end
       end
     end

spec/controllers/projects/notes_controller_spec.rb

@@ -15,14 +15,163 @@ describe Projects::NotesController do
   end
   describe 'GET index' do
-    # It renders the discussion partial for any threaded note
-    # TODO: Test
+    let(:last_fetched_at) { '1487756246' }
+    let(:request_params) do
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        target_type: 'issue',
+        target_id: issue.id,
+        format: 'json'
+      }
+    end
+
+    let(:parsed_response) { JSON.parse(response.body).with_indifferent_access }
+    let(:note_json) { parsed_response[:notes].first }
+
+    before do
+      sign_in(user)
+      project.team << [user, :developer]
+    end
+
+    it 'passes last_fetched_at from headers to NotesFinder' do
+      request.headers['X-Last-Fetched-At'] = last_fetched_at
+
+      expect(NotesFinder).to receive(:new)
+        .with(anything, anything, hash_including(last_fetched_at: last_fetched_at))
+        .and_call_original
+
+      get :index, request_params
+    end
+
+    context 'for a discussion note' do
+      let!(:note) { create(:discussion_note_on_issue, noteable: issue, project: project) }
+
+      it 'includes the ID' do
+        get :index, request_params
+
+        expect(note_json[:id]).to eq(note.id)
+      end
+
+      it 'includes discussion_html' do
+        get :index, request_params
+
+        expect(note_json[:discussion_html]).not_to be_nil
+      end
+
+      it "doesn't include diff_discussion_html" do
+        get :index, request_params
+
+        expect(note_json[:diff_discussion_html]).to be_nil
+      end
+    end
+
+    context 'for a diff discussion note' do
+      let(:project) { create(:project, :repository) }
+      let!(:note) { create(:diff_note_on_merge_request, project: project) }
+
+      let(:params) { request_params.merge(target_type: 'merge_request', target_id: note.noteable_id) }
+
+      it 'includes the ID' do
+        get :index, params
+
+        expect(note_json[:id]).to eq(note.id)
+      end
+
+      it 'includes discussion_html' do
+        get :index, params
+
+        expect(note_json[:discussion_html]).not_to be_nil
+      end
+
+      it 'includes diff_discussion_html' do
+        get :index, params
+
+        expect(note_json[:diff_discussion_html]).not_to be_nil
+      end
+    end
+
+    context 'for a commit note' do
+      let(:project) { create(:project, :repository) }
+      let!(:note) { create(:note_on_commit, project: project) }
+
+      context 'when displayed on a merge request' do
+        let(:merge_request) { create(:merge_request, source_project: project) }
+
+        let(:params) { request_params.merge(target_type: 'merge_request', target_id: merge_request.id) }
+
+        it 'includes the ID' do
+          get :index, params
+
+          expect(note_json[:id]).to eq(note.id)
+        end
+
+        it 'includes discussion_html' do
+          get :index, params
+
+          expect(note_json[:discussion_html]).not_to be_nil
+        end
+
+        it "doesn't include diff_discussion_html" do
+          get :index, params
+
+          expect(note_json[:diff_discussion_html]).to be_nil
+        end
+      end
+
+      context 'when displayed on the commit' do
+        let(:params) { request_params.merge(target_type: 'commit', target_id: note.commit_id) }
+
+        it 'includes the ID' do
+          get :index, params
+
+          expect(note_json[:id]).to eq(note.id)
+        end
+
+        it "doesn't include discussion_html" do
+          get :index, params
+
+          expect(note_json[:discussion_html]).to be_nil
+        end
+
+        it "doesn't include diff_discussion_html" do
+          get :index, params
+
+          expect(note_json[:diff_discussion_html]).to be_nil
+        end
+      end
+    end
+
+    context 'for a regular note' do
+      let!(:note) { create(:note, noteable: issue, project: project) }
+
+      it 'includes the ID' do
+        get :index, request_params
+
+        expect(note_json[:id]).to eq(note.id)
+      end
+
+      it 'includes html' do
+        get :index, request_params
+
+        expect(note_json[:html]).not_to be_nil
+      end
+
+      it "doesn't include discussion_html" do
+        get :index, request_params
+
+        expect(note_json[:discussion_html]).to be_nil
+      end
+
+      it "doesn't include diff_discussion_html" do
+        get :index, request_params
+
+        expect(note_json[:diff_discussion_html]).to be_nil
+      end
+    end
   end
   describe 'POST create' do
-    # Test :type, :new_discussion, :in_reply_to_discussion_id (in_reply_to_id?)
-    # TODO: Test
-
     let(:merge_request) { create(:merge_request) }
     let(:project) { merge_request.source_project }
     let(:request_params) do
@@ -210,31 +359,4 @@ describe Projects::NotesController do
       end
     end
   end
-
-  describe 'GET index' do
-    let(:last_fetched_at) { '1487756246' }
-    let(:request_params) do
-      {
-        namespace_id: project.namespace,
-        project_id: project,
-        target_type: 'issue',
-        target_id: issue.id
-      }
-    end
-
-    before do
-      sign_in(user)
-      project.team << [user, :developer]
-    end
-
-    it 'passes last_fetched_at from headers to NotesFinder' do
-      request.headers['X-Last-Fetched-At'] = last_fetched_at
-
-      expect(NotesFinder).to receive(:new)
-        .with(anything, anything, hash_including(last_fetched_at: last_fetched_at))
-        .and_call_original
-
-      get :index, request_params
-    end
-  end
 end

spec/factories/discussions.rb

-FactoryGirl.define do
-  factory :discussion do
-    # TODO: Implement
-  end
-end