Address review comments

16 files changed, 395 insertions, 299 deletions

diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index 0f9056a4914..90c35e2c7f8 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -25,17 +25,11 @@ FactoryGirl.define do
       end
     end
 
-    factory :discussion_note_on_issue, traits: [:on_issue], class: DiscussionNote do
-      association :project, :repository
-    end
+    factory :discussion_note_on_issue, traits: [:on_issue], class: DiscussionNote
 
-    factory :discussion_note_on_commit, traits: [:on_commit], class: DiscussionNote do
-      association :project, :repository
-    end
+    factory :discussion_note_on_commit, traits: [:on_commit], class: DiscussionNote
 
-    factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote do
-      association :project, :repository
-    end
+    factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote
 
     factory :legacy_diff_note_on_merge_request, traits: [:on_merge_request, :legacy_diff_note], class: LegacyDiffNote do
       association :project, :repository
diff --git a/spec/features/merge_requests/diff_notes_avatars_spec.rb b/spec/features/merge_requests/diff_notes_avatars_spec.rb
index 5769769a5ba..218d95a88b8 100644
--- a/spec/features/merge_requests/diff_notes_avatars_spec.rb
+++ b/spec/features/merge_requests/diff_notes_avatars_spec.rb
@@ -163,9 +163,9 @@ feature 'Diff note avatars', feature: true, js: true do
       end
 
       context 'multiple comments' do
-        let!(:notes) { create_list(:diff_note_on_merge_request, 3, project: project, noteable: merge_request, in_reply_to: note) }
-
         before do
+          create_list(:diff_note_on_merge_request, 3, project: project, noteable: merge_request, in_reply_to: note)
+
           visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, view: view)
 
           wait_for_ajax
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index c107e4c4457..e6f0a3b5920 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -647,14 +647,14 @@ describe Notify do
       shared_examples 'a discussion note email' do |model|
         it_behaves_like 'it should have Gmail Actions links'
 
-        it 'is sent as the author' do
+        it 'is sent to the given recipient as the author' do
           sender = subject.header[:from].addrs[0]
-          expect(sender.display_name).to eq(note_author.name)
-          expect(sender.address).to eq(gitlab_sender)
-        end
 
-        it 'is sent to the given recipient' do
-          is_expected.to deliver_to recipient.notification_email
+          aggregate_failures do
+            expect(sender.display_name).to eq(note_author.name)
+            expect(sender.address).to eq(gitlab_sender)
+            expect(subject).to deliver_to(recipient.notification_email)
+          end
         end
 
         it 'contains the message from the note' do
diff --git a/spec/mailers/previews/notify_preview.rb b/spec/mailers/previews/notify_preview.rb
new file mode 100644
index 00000000000..d9463efcb4e
--- /dev/null
+++ b/spec/mailers/previews/notify_preview.rb
@@ -0,0 +1,115 @@
+class NotifyPreview < ActionMailer::Preview
+  def note_merge_request_email_for_individual_note
+    note_email(:note_merge_request_email) do
+      note = <<-MD.strip_heredoc
+        This is an individual note on a merge request :smiley:
+
+        In this notification email, we expect to see:
+
+        - The note contents (that's what you're looking at)
+        - A link to view this note on Gitlab
+        - An explanation for why the user is receiving this notification
+      MD
+
+      create_note(noteable_type: 'merge_request', noteable_id: merge_request.id, note: note)
+    end
+  end
+
+  def note_merge_request_email_for_discussion
+    note_email(:note_merge_request_email) do
+      note = <<-MD.strip_heredoc
+        This is a new discussion on a merge request :smiley:
+
+        In this notification email, we expect to see:
+
+        - A line saying who started this discussion
+        - The note contents (that's what you're looking at)
+        - A link to view this discussion on Gitlab
+        - An explanation for why the user is receiving this notification
+      MD
+
+      create_note(noteable_type: 'merge_request', noteable_id: merge_request.id, type: 'DiscussionNote', note: note)
+    end
+  end
+
+  def note_merge_request_email_for_diff_discussion
+    note_email(:note_merge_request_email) do
+      note = <<-MD.strip_heredoc
+        This is a new discussion on a merge request :smiley:
+
+        In this notification email, we expect to see:
+
+        - A line saying who started this discussion and on what file
+        - The diff
+        - The note contents (that's what you're looking at)
+        - A link to view this discussion on Gitlab
+        - An explanation for why the user is receiving this notification
+      MD
+
+      position = Gitlab::Diff::Position.new(
+        old_path: "files/ruby/popen.rb",
+        new_path: "files/ruby/popen.rb",
+        old_line: nil,
+        new_line: 14,
+        diff_refs: merge_request.diff_refs
+      )
+
+      create_note(noteable_type: 'merge_request', noteable_id: merge_request.id, type: 'DiffNote', position: position, note: note)
+    end
+  end
+
+  private
+
+  def project
+    @project ||= Project.find_by_full_path('gitlab-org/gitlab-test')
+  end
+
+  def issue
+    @issue ||= project.issues.last
+  end
+
+  def merge_request
+    @merge_request ||= project.merge_requests.find_by(source_branch: 'master', target_branch: 'feature')
+  end
+
+  def commit
+    @commit ||= project.commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d')
+  end
+
+  def user
+    @user ||= User.last
+  end
+
+  def note_body
+    <<-MD.strip_heredoc
+      Hello :smiley:
+
+      We expect a blank line between:
+      - The heading ("Adminstrator started...")
+      - The diff
+    MD
+  end
+
+  def create_note(params)
+    Notes::CreateService.new(project, user, params).execute
+  end
+
+  def note_email(method)
+    cleanup do
+      note = yield
+
+      Notify.public_send(method, user.id, note)
+    end
+  end
+
+  def cleanup
+    email = nil
+
+    ActiveRecord::Base.transaction do
+      email = yield
+      raise ActiveRecord::Rollback
+    end
+
+    email
+  end
+end
diff --git a/spec/models/concerns/discussion_on_diff_spec.rb b/spec/models/concerns/discussion_on_diff_spec.rb
new file mode 100644
index 00000000000..0002a00770f
--- /dev/null
+++ b/spec/models/concerns/discussion_on_diff_spec.rb
@@ -0,0 +1,24 @@
+require 'spec_helper'
+
+describe DiffDiscussion, DiscussionOnDiff, model: true do
+  subject { create(:diff_note_on_merge_request).to_discussion }
+
+  describe "#truncated_diff_lines" do
+    let(:truncated_lines) { subject.truncated_diff_lines }
+
+    context "when diff is greater than allowed number of truncated diff lines " do
+      it "returns fewer lines"  do
+        expect(subject.diff_lines.count).to be > described_class::NUMBER_OF_TRUNCATED_DIFF_LINES
+
+        expect(truncated_lines.count).to be <= described_class::NUMBER_OF_TRUNCATED_DIFF_LINES
+      end
+    end
+
+    context "when some diff lines are meta" do
+      it "returns no meta lines"  do
+        expect(subject.diff_lines).to include(be_meta)
+        expect(truncated_lines).not_to include(be_meta)
+      end
+    end
+  end
+end
diff --git a/spec/models/concerns/noteable_spec.rb b/spec/models/concerns/noteable_spec.rb
index 0a181c008f3..92cc8859a8c 100644
--- a/spec/models/concerns/noteable_spec.rb
+++ b/spec/models/concerns/noteable_spec.rb
@@ -1,14 +1,14 @@
 require 'spec_helper'
 
 describe MergeRequest, Noteable, model: true do
-  let(:merge_request) { create(:merge_request) }
-  let(:project) { merge_request.project }
-  let!(:active_diff_note1) { create(:diff_note_on_merge_request, project: project, noteable: merge_request) }
-  let!(:active_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, in_reply_to: active_diff_note1) }
-  let!(:active_diff_note3) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: active_position2) }
-  let!(:outdated_diff_note1) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: outdated_position) }
-  let!(:outdated_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: outdated_position, in_reply_to: outdated_diff_note1) }
-  let!(:discussion_note1) { create(:discussion_note_on_merge_request, project: project, noteable: merge_request) }
+  let!(:active_diff_note1) { create(:diff_note_on_merge_request) }
+  let(:project) { active_diff_note1.project }
+  subject { active_diff_note1.noteable }
+  let!(:active_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: subject, in_reply_to: active_diff_note1) }
+  let!(:active_diff_note3) { create(:diff_note_on_merge_request, project: project, noteable: subject, position: active_position2) }
+  let!(:outdated_diff_note1) { create(:diff_note_on_merge_request, project: project, noteable: subject, position: outdated_position) }
+  let!(:outdated_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: subject, in_reply_to: outdated_diff_note1) }
+  let!(:discussion_note1) { create(:discussion_note_on_merge_request, project: project, noteable: subject) }
   let!(:discussion_note2) { create(:discussion_note_on_merge_request, in_reply_to: discussion_note1) }
   let!(:commit_diff_note1) { create(:diff_note_on_commit, project: project) }
   let!(:commit_diff_note2) { create(:diff_note_on_commit, project: project, in_reply_to: commit_diff_note1) }
@@ -17,8 +17,8 @@ describe MergeRequest, Noteable, model: true do
   let!(:commit_discussion_note1) { create(:discussion_note_on_commit, project: project) }
   let!(:commit_discussion_note2) { create(:discussion_note_on_commit, in_reply_to: commit_discussion_note1) }
   let!(:commit_discussion_note3) { create(:discussion_note_on_commit, project: project) }
-  let!(:note1) { create(:note, project: project, noteable: merge_request) }
-  let!(:note2) { create(:note, project: project, noteable: merge_request) }
+  let!(:note1) { create(:note, project: project, noteable: subject) }
+  let!(:note2) { create(:note, project: project, noteable: subject) }
 
   let(:active_position2) do
     Gitlab::Diff::Position.new(
@@ -26,7 +26,7 @@ describe MergeRequest, Noteable, model: true do
       new_path: "files/ruby/popen.rb",
       old_line: 16,
       new_line: 22,
-      diff_refs: merge_request.diff_refs
+      diff_refs: subject.diff_refs
     )
   end
 
@@ -41,29 +41,29 @@ describe MergeRequest, Noteable, model: true do
   end
 
   describe '#discussions' do
-    subject { merge_request.discussions }
+    let(:discussions) { subject.discussions }
 
     it 'includes discussions for diff notes, commit diff notes, commit notes, and regular notes' do
-      expect(subject).to eq([
-        DiffDiscussion.new([active_diff_note1, active_diff_note2], merge_request),
-        DiffDiscussion.new([active_diff_note3], merge_request),
-        DiffDiscussion.new([outdated_diff_note1, outdated_diff_note2], merge_request),
-        SimpleDiscussion.new([discussion_note1, discussion_note2], merge_request),
-        DiffDiscussion.new([commit_diff_note1, commit_diff_note2], merge_request),
-        OutOfContextDiscussion.new([commit_note1, commit_note2], merge_request),
-        SimpleDiscussion.new([commit_discussion_note1, commit_discussion_note2], merge_request),
-        SimpleDiscussion.new([commit_discussion_note3], merge_request),
-        IndividualNoteDiscussion.new([note1], merge_request),
-        IndividualNoteDiscussion.new([note2], merge_request)
+      expect(discussions).to eq([
+        DiffDiscussion.new([active_diff_note1, active_diff_note2], subject),
+        DiffDiscussion.new([active_diff_note3], subject),
+        DiffDiscussion.new([outdated_diff_note1, outdated_diff_note2], subject),
+        Discussion.new([discussion_note1, discussion_note2], subject),
+        DiffDiscussion.new([commit_diff_note1, commit_diff_note2], subject),
+        OutOfContextDiscussion.new([commit_note1, commit_note2], subject),
+        Discussion.new([commit_discussion_note1, commit_discussion_note2], subject),
+        Discussion.new([commit_discussion_note3], subject),
+        IndividualNoteDiscussion.new([note1], subject),
+        IndividualNoteDiscussion.new([note2], subject)
       ])
     end
   end
 
   describe '#grouped_diff_discussions' do
-    subject { merge_request.grouped_diff_discussions }
+    let(:grouped_diff_discussions) { subject.grouped_diff_discussions }
 
     it "includes active discussions" do
-      discussions = subject.values.flatten
+      discussions = grouped_diff_discussions.values.flatten
 
       expect(discussions.count).to eq(2)
       expect(discussions.map(&:id)).to eq([active_diff_note1.discussion_id, active_diff_note3.discussion_id])
@@ -74,12 +74,188 @@ describe MergeRequest, Noteable, model: true do
     end
 
     it "doesn't include outdated discussions" do
-      expect(subject.values.flatten.map(&:id)).not_to include(outdated_diff_note1.discussion_id)
+      expect(grouped_diff_discussions.values.flatten.map(&:id)).not_to include(outdated_diff_note1.discussion_id)
     end
 
     it "groups the discussions by line code" do
-      expect(subject[active_diff_note1.line_code].first.id).to eq(active_diff_note1.discussion_id)
-      expect(subject[active_diff_note3.line_code].first.id).to eq(active_diff_note3.discussion_id)
+      expect(grouped_diff_discussions[active_diff_note1.line_code].first.id).to eq(active_diff_note1.discussion_id)
+      expect(grouped_diff_discussions[active_diff_note3.line_code].first.id).to eq(active_diff_note3.discussion_id)
+    end
+  end
+
+  context "discussion status" do
+    let(:first_discussion) { build_stubbed(:discussion_note_on_merge_request, noteable: subject, project: project).to_discussion }
+    let(:second_discussion) { build_stubbed(:discussion_note_on_merge_request, noteable: subject, project: project).to_discussion }
+    let(:third_discussion) { build_stubbed(:discussion_note_on_merge_request, noteable: subject, project: project).to_discussion }
+
+    before do
+      allow(subject).to receive(:resolvable_discussions).and_return([first_discussion, second_discussion, third_discussion])
+    end
+
+    describe "#discussions_resolvable?" do
+      context "when all discussions are unresolvable" do
+        before do
+          allow(first_discussion).to receive(:resolvable?).and_return(false)
+          allow(second_discussion).to receive(:resolvable?).and_return(false)
+          allow(third_discussion).to receive(:resolvable?).and_return(false)
+        end
+
+        it "returns false" do
+          expect(subject.discussions_resolvable?).to be false
+        end
+      end
+
+      context "when some discussions are unresolvable and some discussions are resolvable" do
+        before do
+          allow(first_discussion).to receive(:resolvable?).and_return(true)
+          allow(second_discussion).to receive(:resolvable?).and_return(false)
+          allow(third_discussion).to receive(:resolvable?).and_return(true)
+        end
+
+        it "returns true" do
+          expect(subject.discussions_resolvable?).to be true
+        end
+      end
+
+      context "when all discussions are resolvable" do
+        before do
+          allow(first_discussion).to receive(:resolvable?).and_return(true)
+          allow(second_discussion).to receive(:resolvable?).and_return(true)
+          allow(third_discussion).to receive(:resolvable?).and_return(true)
+        end
+
+        it "returns true" do
+          expect(subject.discussions_resolvable?).to be true
+        end
+      end
+    end
+
+    describe "#discussions_resolved?" do
+      context "when discussions are not resolvable" do
+        before do
+          allow(subject).to receive(:discussions_resolvable?).and_return(false)
+        end
+
+        it "returns false" do
+          expect(subject.discussions_resolved?).to be false
+        end
+      end
+
+      context "when discussions are resolvable" do
+        before do
+          allow(subject).to receive(:discussions_resolvable?).and_return(true)
+
+          allow(first_discussion).to receive(:resolvable?).and_return(true)
+          allow(second_discussion).to receive(:resolvable?).and_return(false)
+          allow(third_discussion).to receive(:resolvable?).and_return(true)
+        end
+
+        context "when all resolvable discussions are resolved" do
+          before do
+            allow(first_discussion).to receive(:resolved?).and_return(true)
+            allow(third_discussion).to receive(:resolved?).and_return(true)
+          end
+
+          it "returns true" do
+            expect(subject.discussions_resolved?).to be true
+          end
+        end
+
+        context "when some resolvable discussions are not resolved" do
+          before do
+            allow(first_discussion).to receive(:resolved?).and_return(true)
+            allow(third_discussion).to receive(:resolved?).and_return(false)
+          end
+
+          it "returns false" do
+            expect(subject.discussions_resolved?).to be false
+          end
+        end
+      end
+    end
+
+    describe "#discussions_to_be_resolved?" do
+      context "when discussions are not resolvable" do
+        before do
+          allow(subject).to receive(:discussions_resolvable?).and_return(false)
+        end
+
+        it "returns false" do
+          expect(subject.discussions_to_be_resolved?).to be false
+        end
+      end
+
+      context "when discussions are resolvable" do
+        before do
+          allow(subject).to receive(:discussions_resolvable?).and_return(true)
+
+          allow(first_discussion).to receive(:resolvable?).and_return(true)
+          allow(second_discussion).to receive(:resolvable?).and_return(false)
+          allow(third_discussion).to receive(:resolvable?).and_return(true)
+        end
+
+        context "when all resolvable discussions are resolved" do
+          before do
+            allow(first_discussion).to receive(:resolved?).and_return(true)
+            allow(third_discussion).to receive(:resolved?).and_return(true)
+          end
+
+          it "returns false" do
+            expect(subject.discussions_to_be_resolved?).to be false
+          end
+        end
+
+        context "when some resolvable discussions are not resolved" do
+          before do
+            allow(first_discussion).to receive(:resolved?).and_return(true)
+            allow(third_discussion).to receive(:resolved?).and_return(false)
+          end
+
+          it "returns true" do
+            expect(subject.discussions_to_be_resolved?).to be true
+          end
+        end
+      end
+    end
+
+    describe "#discussions_to_be_resolved" do
+      before do
+        allow(first_discussion).to receive(:to_be_resolved?).and_return(true)
+        allow(second_discussion).to receive(:to_be_resolved?).and_return(false)
+        allow(third_discussion).to receive(:to_be_resolved?).and_return(false)
+      end
+
+      it 'includes only discussions that need to be resolved' do
+        expect(subject.discussions_to_be_resolved).to eq([first_discussion])
+      end
+    end
+
+    describe '#discussions_can_be_resolved_by?' do
+      let(:user) { build(:user) }
+
+      context 'all discussions can be resolved by the user' do
+        before do
+          allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
+          allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
+          allow(third_discussion).to receive(:can_resolve?).with(user).and_return(true)
+        end
+
+        it 'allows a user to resolve the discussions' do
+          expect(subject.discussions_can_be_resolved_by?(user)).to be(true)
+        end
+      end
+
+      context 'one discussion cannot be resolved by the user' do
+        before do
+          allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
+          allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
+          allow(third_discussion).to receive(:can_resolve?).with(user).and_return(false)
+        end
+
+        it 'allows a user to resolve the discussions' do
+          expect(subject.discussions_can_be_resolved_by?(user)).to be(false)
+        end
+      end
     end
   end
 end
diff --git a/spec/models/concerns/resolvable_discussion_spec.rb b/spec/models/concerns/resolvable_discussion_spec.rb
index 2d2182fbae9..18327fe262d 100644
--- a/spec/models/concerns/resolvable_discussion_spec.rb
+++ b/spec/models/concerns/resolvable_discussion_spec.rb
@@ -5,8 +5,9 @@ describe Discussion, ResolvableDiscussion, models: true do
 
   let(:first_note) { create(:discussion_note_on_merge_request) }
   let(:merge_request) { first_note.noteable }
-  let(:second_note) { create(:discussion_note_on_merge_request, in_reply_to: first_note) }
-  let(:third_note) { create(:discussion_note_on_merge_request) }
+  let(:project) { first_note.project }
+  let(:second_note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) }
+  let(:third_note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
 
   describe "#resolvable?" do
     context "when potentially resolvable" do
diff --git a/spec/models/concerns/resolvable_note_spec.rb b/spec/models/concerns/resolvable_note_spec.rb
index a5a26958410..1503ccdff11 100644
--- a/spec/models/concerns/resolvable_note_spec.rb
+++ b/spec/models/concerns/resolvable_note_spec.rb
@@ -1,15 +1,17 @@
 require 'spec_helper'
 
 describe Note, ResolvableNote, models: true do
-  subject { create(:discussion_note_on_merge_request) }
+  let(:project) { create(:project) }
+  let(:merge_request) { create(:merge_request, source_project: project) }
+  subject { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
 
   context 'resolvability scopes' do
-    let!(:note1) { create(:note) }
-    let!(:note2) { create(:diff_note_on_commit) }
-    let!(:note3) { create(:diff_note_on_merge_request, :resolved) }
-    let!(:note4) { create(:discussion_note_on_merge_request) }
-    let!(:note5) { create(:discussion_note_on_issue) }
-    let!(:note6) { create(:discussion_note_on_merge_request, :system) }
+    let!(:note1) { create(:note, project: project) }
+    let!(:note2) { create(:diff_note_on_commit, project: project) }
+    let!(:note3) { create(:diff_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
+    let!(:note4) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
+    let!(:note5) { create(:discussion_note_on_issue, project: project) }
+    let!(:note6) { create(:discussion_note_on_merge_request, :system, noteable: merge_request, project: project) }
 
     describe '.potentially_resolvable' do
       it 'includes diff and discussion notes on merge requests' do
@@ -38,9 +40,9 @@ describe Note, ResolvableNote, models: true do
 
   describe ".resolve!" do
     let(:current_user) { create(:user) }
-    let!(:commit_note) { create(:diff_note_on_commit) }
-    let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved) }
-    let!(:unresolved_note) { create(:discussion_note_on_merge_request) }
+    let!(:commit_note) { create(:diff_note_on_commit, project: project) }
+    let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
+    let!(:unresolved_note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
 
     before do
       described_class.resolve!(current_user)
@@ -59,7 +61,7 @@ describe Note, ResolvableNote, models: true do
   end
 
   describe ".unresolve!" do
-    let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved) }
+    let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
 
     before do
       described_class.unresolve!
diff --git a/spec/models/diff_discussion_spec.rb b/spec/models/diff_discussion_spec.rb
index ba7dd5280bd..48e7c0a822c 100644
--- a/spec/models/diff_discussion_spec.rb
+++ b/spec/models/diff_discussion_spec.rb
@@ -4,8 +4,10 @@ describe DiffDiscussion, model: true do
   subject { described_class.new([first_note, second_note, third_note]) }
 
   let(:first_note) { create(:diff_note_on_merge_request) }
-  let(:second_note) { create(:diff_note_on_merge_request, in_reply_to: first_note) }
-  let(:third_note) { create(:diff_note_on_merge_request, in_reply_to: first_note) }
+  let(:merge_request) { first_note.noteable }
+  let(:project) { first_note.project }
+  let(:second_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) }
+  let(:third_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) }
 
   describe '#reply_attributes' do
     it 'includes position and original_position' do
@@ -14,23 +16,4 @@ describe DiffDiscussion, model: true do
       expect(attributes[:original_position]).to eq(first_note.original_position.to_json)
     end
   end
-
-  describe "#truncated_diff_lines" do
-    let(:truncated_lines) { subject.truncated_diff_lines }
-
-    context "when diff is greater than allowed number of truncated diff lines " do
-      it "returns fewer lines"  do
-        expect(subject.diff_lines.count).to be > described_class::NUMBER_OF_TRUNCATED_DIFF_LINES
-
-        expect(truncated_lines.count).to be <= described_class::NUMBER_OF_TRUNCATED_DIFF_LINES
-      end
-    end
-
-    context "when some diff lines are meta" do
-      it "returns no meta lines"  do
-        expect(subject.diff_lines).to include(be_meta)
-        expect(truncated_lines).not_to include(be_meta)
-      end
-    end
-  end
 end
diff --git a/spec/models/discussion_note_spec.rb b/spec/models/discussion_note_spec.rb
deleted file mode 100644
index c9dd3d45270..00000000000
--- a/spec/models/discussion_note_spec.rb
+++ /dev/null
@@ -1,4 +0,0 @@
-require 'spec_helper'
-
-describe DiscussionNote, models: true do
-end
diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb
index 718b208e8f8..0221e23ced8 100644
--- a/spec/models/discussion_spec.rb
+++ b/spec/models/discussion_spec.rb
@@ -3,15 +3,15 @@ require 'spec_helper'
 describe Discussion, model: true do
   subject { described_class.new([first_note, second_note, third_note]) }
 
-  let(:first_note) { create(:discussion_note_on_merge_request) }
+  let(:first_note) { create(:diff_note_on_merge_request) }
   let(:merge_request) { first_note.noteable }
-  let(:second_note) { create(:discussion_note_on_merge_request, in_reply_to: first_note) }
-  let(:third_note) { create(:discussion_note_on_merge_request) }
+  let(:second_note) { create(:diff_note_on_merge_request, in_reply_to: first_note) }
+  let(:third_note) { create(:diff_note_on_merge_request) }
 
   describe '.build' do
     it 'returns a discussion of the right type' do
       discussion = described_class.build([first_note, second_note], merge_request)
-      expect(discussion).to be_a(SimpleDiscussion)
+      expect(discussion).to be_a(DiffDiscussion)
       expect(discussion.notes.count).to be(2)
       expect(discussion.first_note).to be(first_note)
       expect(discussion.noteable).to be(merge_request)
@@ -22,8 +22,8 @@ describe Discussion, model: true do
     it 'returns an array of discussions of the right type' do
       discussions = described_class.build_collection([first_note, second_note, third_note], merge_request)
       expect(discussions).to eq([
-        SimpleDiscussion.new([first_note, second_note], merge_request),
-        SimpleDiscussion.new([third_note], merge_request)
+        DiffDiscussion.new([first_note, second_note], merge_request),
+        DiffDiscussion.new([third_note], merge_request)
       ])
     end
   end
diff --git a/spec/models/individual_note_discussion_spec.rb b/spec/models/individual_note_discussion_spec.rb
deleted file mode 100644
index 1f7c2c820e0..00000000000
--- a/spec/models/individual_note_discussion_spec.rb
+++ /dev/null
@@ -1,4 +0,0 @@
-require 'spec_helper'
-
-describe IndividualNoteDiscussion, models: true do
-end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 5d0862abb92..b615d956686 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1224,182 +1224,6 @@ describe MergeRequest, models: true do
     end
   end
 
-  context "discussion status" do
-    let(:first_discussion) { create(:discussion_note_on_merge_request).to_discussion }
-    let(:second_discussion) { create(:discussion_note_on_merge_request).to_discussion }
-    let(:third_discussion) { create(:discussion_note_on_merge_request).to_discussion }
-
-    before do
-      allow(subject).to receive(:resolvable_discussions).and_return([first_discussion, second_discussion, third_discussion])
-    end
-
-    describe "#discussions_resolvable?" do
-      context "when all discussions are unresolvable" do
-        before do
-          allow(first_discussion).to receive(:resolvable?).and_return(false)
-          allow(second_discussion).to receive(:resolvable?).and_return(false)
-          allow(third_discussion).to receive(:resolvable?).and_return(false)
-        end
-
-        it "returns false" do
-          expect(subject.discussions_resolvable?).to be false
-        end
-      end
-
-      context "when some discussions are unresolvable and some discussions are resolvable" do
-        before do
-          allow(first_discussion).to receive(:resolvable?).and_return(true)
-          allow(second_discussion).to receive(:resolvable?).and_return(false)
-          allow(third_discussion).to receive(:resolvable?).and_return(true)
-        end
-
-        it "returns true" do
-          expect(subject.discussions_resolvable?).to be true
-        end
-      end
-
-      context "when all discussions are resolvable" do
-        before do
-          allow(first_discussion).to receive(:resolvable?).and_return(true)
-          allow(second_discussion).to receive(:resolvable?).and_return(true)
-          allow(third_discussion).to receive(:resolvable?).and_return(true)
-        end
-
-        it "returns true" do
-          expect(subject.discussions_resolvable?).to be true
-        end
-      end
-    end
-
-    describe "#discussions_resolved?" do
-      context "when discussions are not resolvable" do
-        before do
-          allow(subject).to receive(:discussions_resolvable?).and_return(false)
-        end
-
-        it "returns false" do
-          expect(subject.discussions_resolved?).to be false
-        end
-      end
-
-      context "when discussions are resolvable" do
-        before do
-          allow(subject).to receive(:discussions_resolvable?).and_return(true)
-
-          allow(first_discussion).to receive(:resolvable?).and_return(true)
-          allow(second_discussion).to receive(:resolvable?).and_return(false)
-          allow(third_discussion).to receive(:resolvable?).and_return(true)
-        end
-
-        context "when all resolvable discussions are resolved" do
-          before do
-            allow(first_discussion).to receive(:resolved?).and_return(true)
-            allow(third_discussion).to receive(:resolved?).and_return(true)
-          end
-
-          it "returns true" do
-            expect(subject.discussions_resolved?).to be true
-          end
-        end
-
-        context "when some resolvable discussions are not resolved" do
-          before do
-            allow(first_discussion).to receive(:resolved?).and_return(true)
-            allow(third_discussion).to receive(:resolved?).and_return(false)
-          end
-
-          it "returns false" do
-            expect(subject.discussions_resolved?).to be false
-          end
-        end
-      end
-    end
-
-    describe "#discussions_to_be_resolved?" do
-      context "when discussions are not resolvable" do
-        before do
-          allow(subject).to receive(:discussions_resolvable?).and_return(false)
-        end
-
-        it "returns false" do
-          expect(subject.discussions_to_be_resolved?).to be false
-        end
-      end
-
-      context "when discussions are resolvable" do
-        before do
-          allow(subject).to receive(:discussions_resolvable?).and_return(true)
-
-          allow(first_discussion).to receive(:resolvable?).and_return(true)
-          allow(second_discussion).to receive(:resolvable?).and_return(false)
-          allow(third_discussion).to receive(:resolvable?).and_return(true)
-        end
-
-        context "when all resolvable discussions are resolved" do
-          before do
-            allow(first_discussion).to receive(:resolved?).and_return(true)
-            allow(third_discussion).to receive(:resolved?).and_return(true)
-          end
-
-          it "returns false" do
-            expect(subject.discussions_to_be_resolved?).to be false
-          end
-        end
-
-        context "when some resolvable discussions are not resolved" do
-          before do
-            allow(first_discussion).to receive(:resolved?).and_return(true)
-            allow(third_discussion).to receive(:resolved?).and_return(false)
-          end
-
-          it "returns true" do
-            expect(subject.discussions_to_be_resolved?).to be true
-          end
-        end
-      end
-    end
-
-    describe "#discussions_to_be_resolved" do
-      before do
-        allow(first_discussion).to receive(:to_be_resolved?).and_return(true)
-        allow(second_discussion).to receive(:to_be_resolved?).and_return(false)
-        allow(third_discussion).to receive(:to_be_resolved?).and_return(false)
-      end
-
-      it 'includes only discussions that need to be resolved' do
-        expect(subject.discussions_to_be_resolved).to eq([first_discussion])
-      end
-    end
-
-    describe '#discussions_can_be_resolved_by?' do
-      let(:user) { build(:user) }
-
-      context 'all discussions can be resolved by the user' do
-        before do
-          allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
-          allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
-          allow(third_discussion).to receive(:can_resolve?).with(user).and_return(true)
-        end
-
-        it 'allows a user to resolve the discussions' do
-          expect(subject.discussions_can_be_resolved_by?(user)).to be(true)
-        end
-      end
-
-      context 'one discussion cannot be resolved by the user' do
-        before do
-          allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
-          allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
-          allow(third_discussion).to receive(:can_resolve?).with(user).and_return(false)
-        end
-
-        it 'allows a user to resolve the discussions' do
-          expect(subject.discussions_can_be_resolved_by?(user)).to be(false)
-        end
-      end
-    end
-  end
-
   describe '#conflicts_can_be_resolved_in_ui?' do
     def create_merge_request(source_branch)
       create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start') do |mr|
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 2c7f6e59b19..3c4bf3f4ddb 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -250,7 +250,7 @@ describe Note, models: true do
     let!(:note2) { create(:discussion_note_on_merge_request, in_reply_to: note) }
     let(:merge_request) { note.noteable }
 
-    it 'returns a discussion with multiple note' do
+    it 'returns a discussion with multiple notes' do
       discussion = merge_request.notes.find_discussion(note.discussion_id)
 
       expect(discussion).not_to be_nil
@@ -379,57 +379,57 @@ describe Note, models: true do
 
   describe '#can_be_discussion_note?' do
     context 'for a note on a merge request' do
-      let(:note) { build(:note_on_merge_request) }
-
       it 'returns true' do
+        note = build(:note_on_merge_request)
+
         expect(note.can_be_discussion_note?).to be_truthy
       end
     end
 
     context 'for a note on an issue' do
-      let(:note) { build(:note_on_issue) }
-
       it 'returns true' do
+        note = build(:note_on_issue)
+
         expect(note.can_be_discussion_note?).to be_truthy
       end
     end
 
     context 'for a note on a commit' do
-      let(:note) { build(:note_on_commit) }
-
       it 'returns true' do
+        note = build(:note_on_commit)
+
         expect(note.can_be_discussion_note?).to be_truthy
       end
     end
 
     context 'for a note on a snippet' do
-      let(:note) { build(:note_on_project_snippet) }
-
       it 'returns true' do
+        note = build(:note_on_project_snippet)
+
         expect(note.can_be_discussion_note?).to be_truthy
       end
     end
 
     context 'for a diff note on merge request' do
-      let(:note) { build(:diff_note_on_merge_request) }
-
       it 'returns false' do
+        note = build(:diff_note_on_merge_request)
+
         expect(note.can_be_discussion_note?).to be_falsey
       end
     end
 
     context 'for a diff note on commit' do
-      let(:note) { build(:diff_note_on_commit) }
-
       it 'returns false' do
+        note = build(:diff_note_on_commit)
+
         expect(note.can_be_discussion_note?).to be_falsey
       end
     end
 
     context 'for a discussion note' do
-      let(:note) { build(:discussion_note_on_merge_request) }
-
       it 'returns false' do
+        note = build(:discussion_note_on_merge_request)
+
         expect(note.can_be_discussion_note?).to be_falsey
       end
     end
diff --git a/spec/models/out_of_context_discussion_spec.rb b/spec/models/out_of_context_discussion_spec.rb
deleted file mode 100644
index 1bcd118855e..00000000000
--- a/spec/models/out_of_context_discussion_spec.rb
+++ /dev/null
@@ -1,4 +0,0 @@
-require 'spec_helper'
-
-describe OutOfContextDiscussion, model: true do
-end
diff --git a/spec/models/simple_discussion_spec.rb b/spec/models/simple_discussion_spec.rb
deleted file mode 100644
index 1a342fbc740..00000000000
--- a/spec/models/simple_discussion_spec.rb
+++ /dev/null
@@ -1,11 +0,0 @@
-require 'spec_helper'
-
-describe SimpleDiscussion, model: true do
-  subject { create(:discussion_note_on_issue).to_discussion }
-
-  describe '#reply_attributes' do
-    it 'includes discussion_id' do
-      expect(subject.reply_attributes[:discussion_id]).to eq(subject.id)
-    end
-  end
-end