From 1bcb1ef85e63478c8b48524c8a761f9819673f50 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 25 Apr 2016 13:19:57 +0200
Subject: [PATCH 01/16] Update specs for creating new note without access

---
 spec/requests/api/notes_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb
index ed1ed5aeb95..beb29a68692 100644
--- a/spec/requests/api/notes_spec.rb
+++ b/spec/requests/api/notes_spec.rb
@@ -258,8 +258,8 @@ describe API::API, api: true  do
              body: 'Hi!'
       end
 
-      it 'responds with 500' do
-        expect(response.status).to eq 500
+      it 'responds with resource not found error' do
+        expect(response.status).to eq 404
       end
 
       it 'does not create new note' do

From f739a4e8718289f5a16d216e93021cf773c547bd Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 27 Apr 2016 10:00:19 +0200
Subject: [PATCH 02/16] Remove reduntant note validation from create service

---
 app/services/notes/create_service.rb | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index 01586994813..2bb312bb252 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -5,8 +5,6 @@ module Notes
       note.author = current_user
       note.system = false
 
-      return unless valid_project?(note)
-
       if note.save
         # Finish the harder work in the background
         NewNoteWorker.perform_in(2.seconds, note.id, params)
@@ -15,14 +13,5 @@ module Notes
 
       note
     end
-
-    private
-
-    def valid_project?(note)
-      return false unless project
-      return true if note.for_commit?
-
-      note.noteable.try(:project) == project
-    end
   end
 end

From c31a296c94c48d865ce3171ccea9c48aa691466c Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 25 Apr 2016 14:28:23 +0200
Subject: [PATCH 03/16] Make note invalid if noteable project is different

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/15577
---
 app/models/note.rb | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/app/models/note.rb b/app/models/note.rb
index 55b98557244..8743b5eb472 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -33,6 +33,12 @@ class Note < ActiveRecord::Base
   validates :commit_id, presence: true, if: ->(n) { n.noteable_type == 'Commit' }
   validates :author, presence: true
 
+  validate do |note|
+    unless note.noteable.project == project
+      errors.add(:invalid_project, 'Note and noteable project mismatch')
+    end
+  end
+
   mount_uploader :attachment, AttachmentUploader
 
   # Scopes

From 87c44b0e91959439014fb8c923f3febc039d1cd0 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Apr 2016 09:35:03 +0200
Subject: [PATCH 04/16] Improve note validation for project mismatch

---
 app/models/note.rb | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/app/models/note.rb b/app/models/note.rb
index 8743b5eb472..02a98a37117 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -29,13 +29,15 @@ class Note < ActiveRecord::Base
   # Attachments are deprecated and are handled by Markdown uploader
   validates :attachment, file_size: { maximum: :max_attachment_size }
 
-  validates :noteable_id, presence: true, if: ->(n) { n.noteable_type.present? && n.noteable_type != 'Commit' }
-  validates :commit_id, presence: true, if: ->(n) { n.noteable_type == 'Commit' }
+  validates :noteable_id, presence: true, unless: :for_commit?
+  validates :commit_id, presence: true, if: :for_commit?
   validates :author, presence: true
 
-  validate do |note|
-    unless note.noteable.project == project
-      errors.add(:invalid_project, 'Note and noteable project mismatch')
+  with_options unless: :for_commit? do
+    validate do |note|
+      unless note.noteable.try(:project) == project
+        errors.add(:invalid_project, 'Note and noteable project mismatch')
+      end
     end
   end
 

From bf0b51d252e049404a49787c18e5c88071006e15 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Apr 2016 11:11:58 +0200
Subject: [PATCH 05/16] Update note factory to include noteable association

---
 spec/factories/notes.rb  | 18 ++++++------------
 spec/models/note_spec.rb |  6 +++---
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index 26719f2652c..7e9378ab625 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -4,13 +4,14 @@ include ActionDispatch::TestProcess
 
 FactoryGirl.define do
   factory :note do
-    project
     note "Note"
     author
+    noteable { create(:issue) }
+    project { noteable.project }
 
+    factory :note_on_issue,              aliases: [:votable_note]
     factory :note_on_commit,             traits: [:on_commit]
     factory :note_on_commit_diff,        traits: [:on_commit, :on_diff], class: LegacyDiffNote
-    factory :note_on_issue,              traits: [:on_issue], aliases: [:votable_note]
     factory :note_on_merge_request,      traits: [:on_merge_request]
     factory :note_on_merge_request_diff, traits: [:on_merge_request, :on_diff], class: LegacyDiffNote
     factory :note_on_project_snippet,    traits: [:on_project_snippet]
@@ -20,6 +21,7 @@ FactoryGirl.define do
 
     trait :on_commit do
       project
+      noteable nil
       commit_id RepoHelpers.sample_commit.id
       noteable_type "Commit"
     end
@@ -29,19 +31,11 @@ FactoryGirl.define do
     end
 
     trait :on_merge_request do
-      project
-      noteable_id 1
-      noteable_type "MergeRequest"
-    end
-
-    trait :on_issue do
-      noteable_id 1
-      noteable_type "Issue"
+      noteable { create(:merge_request) }
     end
 
     trait :on_project_snippet do
-      noteable_id 1
-      noteable_type "Snippet"
+      noteable { create(:snippet) }
     end
 
     trait :system do
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 5d916f0e6a6..fe13c06b1e0 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -89,8 +89,8 @@ describe Note, models: true do
   end
 
   describe "#all_references" do
-    let!(:note1) { create(:note) }
-    let!(:note2) { create(:note) }
+    let!(:note1) { create(:note_on_issue) }
+    let!(:note2) { create(:note_on_issue) }
 
     it "reads the rendered note body from the cache" do
       expect(Banzai::Renderer).to receive(:render).with(note1.note, pipeline: :note, cache_key: [note1, "note"], project: note1.project)
@@ -102,7 +102,7 @@ describe Note, models: true do
   end
 
   describe '.search' do
-    let(:note) { create(:note, note: 'WoW') }
+    let(:note) { create(:note_on_issue, note: 'WoW') }
 
     it 'returns notes with matching content' do
       expect(described_class.search(note.note)).to eq([note])

From 57b551a19f18d8da78175b4de6098d5517cde49f Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Apr 2016 11:36:50 +0200
Subject: [PATCH 06/16] Remove redundant `with_options` from note validators

---
 app/models/note.rb | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/app/models/note.rb b/app/models/note.rb
index 02a98a37117..f10446ca45f 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -33,11 +33,9 @@ class Note < ActiveRecord::Base
   validates :commit_id, presence: true, if: :for_commit?
   validates :author, presence: true
 
-  with_options unless: :for_commit? do
-    validate do |note|
-      unless note.noteable.try(:project) == project
-        errors.add(:invalid_project, 'Note and noteable project mismatch')
-      end
+  validate unless: :for_commit? do |note|
+    unless note.noteable.try(:project) == project
+      errors.add(:invalid_project, 'Note and noteable project mismatch')
     end
   end
 

From 0e613db76d8d6e8521a3b4d546f552c8d184ffa1 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Apr 2016 11:59:41 +0200
Subject: [PATCH 07/16] Add more validation tests for note model

---
 spec/models/note_spec.rb | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index fe13c06b1e0..d90b54464cd 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -12,6 +12,33 @@ describe Note, models: true do
   describe 'validation' do
     it { is_expected.to validate_presence_of(:note) }
     it { is_expected.to validate_presence_of(:project) }
+
+    context 'when note is comment on commit' do
+      before { allow(subject).to receive(:for_commit?).and_return(true) }
+
+      it { is_expected.to validate_presence_of(:commit_id) }
+      it { is_expected.to_not validate_presence_of(:noteable_id) }
+    end
+
+    context 'when note is not comment on commit' do
+      before { allow(subject).to receive(:for_commit?).and_return(false) }
+
+      it { is_expected.to_not validate_presence_of(:commit_id) }
+      it { is_expected.to validate_presence_of(:noteable_id) }
+    end
+
+    context 'when noteable and note project is different' do
+      subject do
+        build(:note, noteable: create(:issue), project: create(:project))
+      end
+
+      it { is_expected.to be_invalid }
+    end
+
+    context 'when noteable and note project is the same one' do
+      subject { create(:note) }
+      it { is_expected.to be_valid }
+    end
   end
 
   describe "Commit notes" do

From 21d0cddd456c03e776a8b30e4695ede94c400792 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Apr 2016 13:50:48 +0200
Subject: [PATCH 08/16] Do not override foreign attributes in note factory

---
 app/models/note.rb      | 2 +-
 spec/factories/notes.rb | 9 ++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/app/models/note.rb b/app/models/note.rb
index f10446ca45f..e3e522a8d0f 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -34,7 +34,7 @@ class Note < ActiveRecord::Base
   validates :author, presence: true
 
   validate unless: :for_commit? do |note|
-    unless note.noteable.try(:project) == project
+    unless note.noteable.try(:project) == note.project
       errors.add(:invalid_project, 'Note and noteable project mismatch')
     end
   end
diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index 7e9378ab625..cabab0c2207 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -4,10 +4,10 @@ include ActionDispatch::TestProcess
 
 FactoryGirl.define do
   factory :note do
+    project
     note "Note"
     author
-    noteable { create(:issue) }
-    project { noteable.project }
+    noteable { create(:issue, project: project) }
 
     factory :note_on_issue,              aliases: [:votable_note]
     factory :note_on_commit,             traits: [:on_commit]
@@ -20,7 +20,6 @@ FactoryGirl.define do
     factory :upvote_note,                traits: [:award, :upvote]
 
     trait :on_commit do
-      project
       noteable nil
       commit_id RepoHelpers.sample_commit.id
       noteable_type "Commit"
@@ -31,11 +30,11 @@ FactoryGirl.define do
     end
 
     trait :on_merge_request do
-      noteable { create(:merge_request) }
+      noteable { create(:merge_request, project: project) }
     end
 
     trait :on_project_snippet do
-      noteable { create(:snippet) }
+      noteable { create(:snippet, project: project) }
     end
 
     trait :system do

From e558edd1ce47c3c056dd95c0eba8fd811ee749c7 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 26 Apr 2016 13:52:18 +0200
Subject: [PATCH 09/16] Update specs to carry out changes in note factory

---
 features/steps/dashboard/todos.rb             |   2 +-
 features/steps/project/issues/issues.rb       |   2 +-
 spec/factories/notes.rb                       |  21 ++-
 spec/features/issues/note_polling_spec.rb     |   6 +-
 spec/features/issues_spec.rb                  |   2 +-
 spec/features/notes_on_merge_requests_spec.rb |  10 +-
 .../participants_autocomplete_spec.rb         |   6 +-
 spec/features/task_lists_spec.rb              |   5 +-
 spec/lib/gitlab/note_data_builder_spec.rb     |  71 +++++++--
 spec/models/concerns/issuable_spec.rb         |   7 +-
 spec/models/merge_request_spec.rb             |   9 +-
 spec/models/note_spec.rb                      |  10 +-
 .../project_services/hipchat_service_spec.rb  | 147 +++++++++++-------
 .../project_services/slack_service_spec.rb    |  68 +++++---
 .../merge_when_build_succeeds_service_spec.rb |  10 +-
 spec/services/system_note_service_spec.rb     |  12 +-
 16 files changed, 258 insertions(+), 130 deletions(-)

diff --git a/features/steps/dashboard/todos.rb b/features/steps/dashboard/todos.rb
index d3b6c7f6a15..bd8a270202e 100644
--- a/features/steps/dashboard/todos.rb
+++ b/features/steps/dashboard/todos.rb
@@ -20,7 +20,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
   step 'I have todos' do
     create(:todo, user: current_user, project: project, author: mary_jane, target: issue, action: Todo::MENTIONED)
     create(:todo, user: current_user, project: project, author: john_doe, target: issue, action: Todo::ASSIGNED)
-    note = create(:note, author: john_doe, noteable: issue, note: "#{current_user.to_reference} Wdyt?")
+    note = create(:note, author: john_doe, noteable: issue, note: "#{current_user.to_reference} Wdyt?", project: project)
     create(:todo, user: current_user, project: project, author: john_doe, target: issue, action: Todo::MENTIONED, note: note)
     create(:todo, user: current_user, project: project, author: john_doe, target: merge_request, action: Todo::ASSIGNED)
   end
diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb
index f2c68f007ef..5cd431e05d5 100644
--- a/features/steps/project/issues/issues.rb
+++ b/features/steps/project/issues/issues.rb
@@ -348,7 +348,7 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps
 
   step 'another user adds a comment with text "Yay!" to issue "Release 0.4"' do
     issue = Issue.find_by!(title: 'Release 0.4')
-    create(:note_on_issue, noteable: issue,  note: 'Yay!')
+    create(:note_on_issue, noteable: issue, project: project, note: 'Yay!')
   end
 
   step 'I should see a new comment with text "Yay!"' do
diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index cabab0c2207..2940ac342a2 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -9,7 +9,7 @@ FactoryGirl.define do
     author
     noteable { create(:issue, project: project) }
 
-    factory :note_on_issue,              aliases: [:votable_note]
+    factory :note_on_issue,              traits: [:on_issue], aliases: [:votable_note]
     factory :note_on_commit,             traits: [:on_commit]
     factory :note_on_commit_diff,        traits: [:on_commit, :on_diff], class: LegacyDiffNote
     factory :note_on_merge_request,      traits: [:on_merge_request]
@@ -19,24 +19,33 @@ FactoryGirl.define do
     factory :downvote_note,              traits: [:award, :downvote]
     factory :upvote_note,                traits: [:award, :upvote]
 
+    trait :on_issue do
+      noteable_type 'Issue'
+    end
+
     trait :on_commit do
       noteable nil
       commit_id RepoHelpers.sample_commit.id
       noteable_type "Commit"
     end
 
-    trait :on_diff do
-      line_code "0_184_184"
-    end
-
     trait :on_merge_request do
-      noteable { create(:merge_request, project: project) }
+      noteable_type 'MergeRequest'
+      noteable do
+        create(:merge_request, source_project: project,
+                               target_project: project)
+      end
     end
 
     trait :on_project_snippet do
+      noteable_type 'Snippet'
       noteable { create(:snippet, project: project) }
     end
 
+    trait :on_diff do
+      line_code "0_184_184"
+    end
+
     trait :system do
       system true
     end
diff --git a/spec/features/issues/note_polling_spec.rb b/spec/features/issues/note_polling_spec.rb
index e4efdbe2421..9f521263e4c 100644
--- a/spec/features/issues/note_polling_spec.rb
+++ b/spec/features/issues/note_polling_spec.rb
@@ -9,8 +9,12 @@ feature 'Issue notes polling' do
   end
 
   scenario 'Another user adds a comment to an issue', js: true do
-    note = create(:note_on_issue, noteable: issue, note: 'Looks good!')
+    note = create(:note_on_issue, noteable: issue,
+                                  project: project,
+                                  note: 'Looks good!')
+
     page.execute_script('notes.refresh();')
+
     expect(page).to have_selector("#note_#{note.id}", text: 'Looks good!')
   end
 end
diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb
index 749ee01890c..9271964166a 100644
--- a/spec/features/issues_spec.rb
+++ b/spec/features/issues_spec.rb
@@ -125,7 +125,7 @@ describe 'Issues', feature: true do
   describe 'Issue info' do
     it 'excludes award_emoji from comment count' do
       issue = create(:issue, author: @user, assignee: @user, project: project, title: 'foobar')
-      create(:upvote_note, noteable: issue)
+      create(:upvote_note, noteable: issue, project: project)
 
       visit namespace_project_issues_path(project.namespace, project, assignee_id: @user.id)
 
diff --git a/spec/features/notes_on_merge_requests_spec.rb b/spec/features/notes_on_merge_requests_spec.rb
index 9e9fec01943..2835cf44494 100644
--- a/spec/features/notes_on_merge_requests_spec.rb
+++ b/spec/features/notes_on_merge_requests_spec.rb
@@ -19,10 +19,14 @@ describe 'Comments', feature: true do
   end
 
   describe 'On a merge request', js: true, feature: true do
-    let!(:merge_request) { create(:merge_request) }
-    let!(:project) { merge_request.source_project }
+    let!(:project) { create(:project) }
+    let!(:merge_request) do
+      create(:merge_request, source_project: project, target_project: project)
+    end
+
     let!(:note) do
-      create(:note_on_merge_request, :with_attachment, project: project)
+      create(:note_on_merge_request, :with_attachment, noteable: merge_request,
+                                                       project: project)
     end
 
     before do
diff --git a/spec/features/participants_autocomplete_spec.rb b/spec/features/participants_autocomplete_spec.rb
index 1adab7e9c6c..c7c00a3266a 100644
--- a/spec/features/participants_autocomplete_spec.rb
+++ b/spec/features/participants_autocomplete_spec.rb
@@ -32,7 +32,8 @@ feature 'Member autocomplete', feature: true do
   context 'adding a new note on a Issue', js: true do
     before do
       issue = create(:issue, author: author, project: project)
-      create(:note, note: 'Ultralight Beam', noteable: issue, author: participant)
+      create(:note, note: 'Ultralight Beam', noteable: issue,
+                    project: project, author: participant)
       visit_issue(project, issue)
     end
 
@@ -47,7 +48,8 @@ feature 'Member autocomplete', feature: true do
   context 'adding a new note on a Merge Request ', js: true do
     before do
       merge = create(:merge_request, source_project: project, target_project: project, author: author)
-      create(:note, note: 'Ultralight Beam', noteable: merge, author: participant)
+      create(:note, note: 'Ultralight Beam', noteable: merge,
+                    project: project, author: participant)
       visit_merge_request(project, merge)
     end
 
diff --git a/spec/features/task_lists_spec.rb b/spec/features/task_lists_spec.rb
index b7368cca29d..6ed279ef9be 100644
--- a/spec/features/task_lists_spec.rb
+++ b/spec/features/task_lists_spec.rb
@@ -75,7 +75,10 @@ feature 'Task Lists', feature: true do
 
   describe 'for Notes' do
     let!(:issue) { create(:issue, author: user, project: project) }
-    let!(:note)  { create(:note, note: markdown, noteable: issue, author: user) }
+    let!(:note) do
+      create(:note, note: markdown, noteable: issue,
+                    project: project, author: user)
+    end
 
     it 'renders for note body' do
       visit_issue(project, issue)
diff --git a/spec/lib/gitlab/note_data_builder_spec.rb b/spec/lib/gitlab/note_data_builder_spec.rb
index f093d0a0d8b..82823e7f790 100644
--- a/spec/lib/gitlab/note_data_builder_spec.rb
+++ b/spec/lib/gitlab/note_data_builder_spec.rb
@@ -9,7 +9,8 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
   before(:each) do
     expect(data).to have_key(:object_attributes)
     expect(data[:object_attributes]).to have_key(:url)
-    expect(data[:object_attributes][:url]).to eq(Gitlab::UrlBuilder.build(note))
+    expect(data[:object_attributes][:url])
+      .to eq(Gitlab::UrlBuilder.build(note))
     expect(data[:object_kind]).to eq('note')
     expect(data[:user]).to eq(user.hook_attrs)
   end
@@ -37,13 +38,21 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
   end
 
   describe 'When asking for a note on issue' do
-    let(:issue) { create(:issue, created_at: fixed_time, updated_at: fixed_time) }
-    let(:note) { create(:note_on_issue, noteable_id: issue.id, project: project) }
+    let(:issue) do
+      create(:issue, created_at: fixed_time, updated_at: fixed_time,
+                     project: project)
+    end
+
+    let(:note) do
+      create(:note_on_issue, noteable_id: issue.id, project: project)
+    end
 
     it 'returns the note and issue-specific data' do
       expect(data).to have_key(:issue)
-      expect(data[:issue].except('updated_at')).to eq(issue.hook_attrs.except('updated_at'))
-      expect(data[:issue]['updated_at']).to be > issue.hook_attrs['updated_at']
+      expect(data[:issue].except('updated_at'))
+        .to eq(issue.hook_attrs.except('updated_at'))
+      expect(data[:issue]['updated_at'])
+        .to be > issue.hook_attrs['updated_at']
     end
 
     include_examples 'project hook data'
@@ -51,13 +60,23 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
   end
 
   describe 'When asking for a note on merge request' do
-    let(:merge_request) { create(:merge_request, created_at: fixed_time, updated_at: fixed_time) }
-    let(:note) { create(:note_on_merge_request, noteable_id: merge_request.id, project: project) }
+    let(:merge_request) do
+      create(:merge_request, created_at: fixed_time,
+                             updated_at: fixed_time,
+                             source_project: project)
+    end
+
+    let(:note) do
+      create(:note_on_merge_request, noteable_id: merge_request.id,
+                                     project: project)
+    end
 
     it 'returns the note and merge request data' do
       expect(data).to have_key(:merge_request)
-      expect(data[:merge_request].except('updated_at')).to eq(merge_request.hook_attrs.except('updated_at'))
-      expect(data[:merge_request]['updated_at']).to be > merge_request.hook_attrs['updated_at']
+      expect(data[:merge_request].except('updated_at'))
+        .to eq(merge_request.hook_attrs.except('updated_at'))
+      expect(data[:merge_request]['updated_at'])
+        .to be > merge_request.hook_attrs['updated_at']
     end
 
     include_examples 'project hook data'
@@ -65,13 +84,22 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
   end
 
   describe 'When asking for a note on merge request diff' do
-    let(:merge_request) { create(:merge_request, created_at: fixed_time, updated_at: fixed_time) }
-    let(:note) { create(:note_on_merge_request_diff, noteable_id: merge_request.id, project: project) }
+    let(:merge_request) do
+      create(:merge_request, created_at: fixed_time, updated_at: fixed_time,
+                             source_project: project)
+    end
+
+    let(:note) do
+      create(:note_on_merge_request_diff, noteable_id: merge_request.id,
+                                          project: project)
+    end
 
     it 'returns the note and merge request diff data' do
       expect(data).to have_key(:merge_request)
-      expect(data[:merge_request].except('updated_at')).to eq(merge_request.hook_attrs.except('updated_at'))
-      expect(data[:merge_request]['updated_at']).to be > merge_request.hook_attrs['updated_at']
+      expect(data[:merge_request].except('updated_at'))
+        .to eq(merge_request.hook_attrs.except('updated_at'))
+      expect(data[:merge_request]['updated_at'])
+        .to be > merge_request.hook_attrs['updated_at']
     end
 
     include_examples 'project hook data'
@@ -79,13 +107,22 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
   end
 
   describe 'When asking for a note on project snippet' do
-    let!(:snippet) { create(:project_snippet, created_at: fixed_time, updated_at: fixed_time) }
-    let!(:note) { create(:note_on_project_snippet, noteable_id: snippet.id, project: project) }
+    let!(:snippet) do
+      create(:project_snippet, created_at: fixed_time, updated_at: fixed_time,
+                               project: project)
+    end
+
+    let!(:note) do
+      create(:note_on_project_snippet, noteable_id: snippet.id,
+                                       project: project)
+    end
 
     it 'returns the note and project snippet data' do
       expect(data).to have_key(:snippet)
-      expect(data[:snippet].except('updated_at')).to eq(snippet.hook_attrs.except('updated_at'))
-      expect(data[:snippet]['updated_at']).to be > snippet.hook_attrs['updated_at']
+      expect(data[:snippet].except('updated_at'))
+        .to eq(snippet.hook_attrs.except('updated_at'))
+      expect(data[:snippet]['updated_at'])
+        .to be > snippet.hook_attrs['updated_at']
     end
 
     include_examples 'project hook data'
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 583827cdf42..331ebaf4ac4 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -189,7 +189,6 @@ describe Issue, "Issuable" do
     let(:data) { issue.to_hook_data(user) }
     let(:project) { issue.project }
 
-
     it "returns correct hook data" do
       expect(data[:object_kind]).to eq("issue")
       expect(data[:user]).to eq(user.hook_attrs)
@@ -230,10 +229,8 @@ describe Issue, "Issuable" do
 
   describe "votes" do
     before do
-      author = create :user
-      project = create :empty_project
-      issue.notes.awards.create!(note: "thumbsup", author: author, project: project)
-      issue.notes.awards.create!(note: "thumbsdown", author: author, project: project)
+      issue.notes.awards.create!(note: "thumbsup", author: user, project: issue.project)
+      issue.notes.awards.create!(note: "thumbsdown", author: user, project: issue.project)
     end
 
     it "returns correct values" do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index e269ff26a04..20e383e03b4 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -119,7 +119,9 @@ describe MergeRequest, models: true do
 
     before do
       allow(merge_request).to receive(:commits) { [merge_request.source_project.repository.commit] }
-      create(:note, commit_id: merge_request.commits.first.id, noteable_type: 'Commit', project: merge_request.project)
+      create(:note_on_commit, commit_id: merge_request.commits.first.id,
+                              noteable_type: 'Commit',
+                              project: merge_request.project)
       create(:note, noteable: merge_request, project: merge_request.project)
     end
 
@@ -129,7 +131,10 @@ describe MergeRequest, models: true do
     end
 
     it "should include notes for commits from target project as well" do
-      create(:note, commit_id: merge_request.commits.first.id, noteable_type: 'Commit', project: merge_request.target_project)
+      create(:note_on_commit, commit_id: merge_request.commits.first.id,
+                              noteable_type: 'Commit',
+                              project: merge_request.target_project)
+
       expect(merge_request.commits).not_to be_empty
       expect(merge_request.mr_and_commit_notes.count).to eq(3)
     end
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index d90b54464cd..20d40c47aa6 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -202,12 +202,18 @@ describe Note, models: true do
     let(:merge_request) { create :merge_request }
 
     it "converts aliases to actual name" do
-      note = create(:note, note: ":+1:", noteable: merge_request)
+      note = create(:note, note: ":+1:",
+                           noteable: merge_request,
+                           project: merge_request.project)
+
       expect(note.reload.note).to eq("thumbsup")
     end
 
     it "is not an award emoji when comment is on a diff" do
-      note = create(:note_on_merge_request_diff, note: ":blowfish:", noteable: merge_request, line_code: "11d5d2e667e9da4f7f610f81d86c974b146b13bd_0_2")
+      note = create(:note_on_merge_request_diff, note: ":blowfish:",
+                                                 noteable: merge_request,
+                                                 project: merge_request.project,
+                                                 line_code: "11d5d2e667e9da4f7f610f81d86c974b146b13bd_0_2")
       note = note.reload
 
       expect(note.note).to eq(":blowfish:")
diff --git a/spec/models/project_services/hipchat_service_spec.rb b/spec/models/project_services/hipchat_service_spec.rb
index f887ed4bafc..7013ffbf3e1 100644
--- a/spec/models/project_services/hipchat_service_spec.rb
+++ b/spec/models/project_services/hipchat_service_spec.rb
@@ -176,86 +176,117 @@ describe HipchatService, models: true do
     context "Note events" do
       let(:user) { create(:user) }
       let(:project) { create(:project, creator_id: user.id) }
-      let(:issue)         { create(:issue, project: project) }
-      let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
-      let(:snippet)       { create(:project_snippet, project: project) }
-      let(:commit_note) { create(:note_on_commit, author: user, project: project, commit_id: project.repository.commit.id, note: 'a comment on a commit') }
-      let(:merge_request_note) { create(:note_on_merge_request, noteable_id: merge_request.id, note: "merge request note") }
-      let(:issue_note) { create(:note_on_issue, noteable_id: issue.id, note: "issue note")}
-      let(:snippet_note) { create(:note_on_project_snippet, noteable_id: snippet.id, note: "snippet note") }
 
-      it "should call Hipchat API for commit comment events" do
-        data = Gitlab::NoteDataBuilder.build(commit_note, user)
-        hipchat.execute(data)
+      context 'when commit comment event triggered' do
+        let(:commit_note) do
+          create(:note_on_commit, author: user, project: project,
+                                  commit_id: project.repository.commit.id,
+                                  note: 'a comment on a commit')
+        end
 
-        expect(WebMock).to have_requested(:post, api_url).once
+        it "should call Hipchat API for commit comment events" do
+          data = Gitlab::NoteDataBuilder.build(commit_note, user)
+          hipchat.execute(data)
 
-        message = hipchat.send(:create_message, data)
+          expect(WebMock).to have_requested(:post, api_url).once
 
-        obj_attr = data[:object_attributes]
-        commit_id = Commit.truncate_sha(data[:commit][:id])
-        title = hipchat.send(:format_title, data[:commit][:message])
+          message = hipchat.send(:create_message, data)
 
-        expect(message).to eq("#{user.name} commented on " \
-            "<a href=\"#{obj_attr[:url]}\">commit #{commit_id}</a> in " \
-            "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
-            "#{title}" \
-            "<pre>a comment on a commit</pre>")
+          obj_attr = data[:object_attributes]
+          commit_id = Commit.truncate_sha(data[:commit][:id])
+          title = hipchat.send(:format_title, data[:commit][:message])
+
+          expect(message).to eq("#{user.name} commented on " \
+              "<a href=\"#{obj_attr[:url]}\">commit #{commit_id}</a> in " \
+              "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
+              "#{title}" \
+              "<pre>a comment on a commit</pre>")
+        end
       end
 
-      it "should call Hipchat API for merge request comment events" do
-        data = Gitlab::NoteDataBuilder.build(merge_request_note, user)
-        hipchat.execute(data)
+      context 'when merge request comment event triggered' do
+        let(:merge_request) do
+          create(:merge_request, source_project: project,
+                                 target_project: project)
+        end
 
-        expect(WebMock).to have_requested(:post, api_url).once
+        let(:merge_request_note) do
+          create(:note_on_merge_request, noteable_id: merge_request.id,
+                                         project: project,
+                                         note: "merge request note")
+        end
 
-        message = hipchat.send(:create_message, data)
+        it "should call Hipchat API for merge request comment events" do
+          data = Gitlab::NoteDataBuilder.build(merge_request_note, user)
+          hipchat.execute(data)
 
-        obj_attr = data[:object_attributes]
-        merge_id = data[:merge_request]['iid']
-        title = data[:merge_request]['title']
+          expect(WebMock).to have_requested(:post, api_url).once
 
-        expect(message).to eq("#{user.name} commented on " \
-            "<a href=\"#{obj_attr[:url]}\">merge request !#{merge_id}</a> in " \
-            "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
-            "<b>#{title}</b>" \
-            "<pre>merge request note</pre>")
+          message = hipchat.send(:create_message, data)
+
+          obj_attr = data[:object_attributes]
+          merge_id = data[:merge_request]['iid']
+          title = data[:merge_request]['title']
+
+          expect(message).to eq("#{user.name} commented on " \
+              "<a href=\"#{obj_attr[:url]}\">merge request !#{merge_id}</a> in " \
+              "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
+              "<b>#{title}</b>" \
+              "<pre>merge request note</pre>")
+        end
       end
 
-      it "should call Hipchat API for issue comment events" do
-        data = Gitlab::NoteDataBuilder.build(issue_note, user)
-        hipchat.execute(data)
+      context 'when issue comment event triggered' do
+        let(:issue) { create(:issue, project: project) }
+        let(:issue_note) do
+          create(:note_on_issue, noteable_id: issue.id, project: project,
+                                 note: "issue note")
+        end
 
-        message = hipchat.send(:create_message, data)
+        it "should call Hipchat API for issue comment events" do
+          data = Gitlab::NoteDataBuilder.build(issue_note, user)
+          hipchat.execute(data)
 
-        obj_attr = data[:object_attributes]
-        issue_id = data[:issue]['iid']
-        title = data[:issue]['title']
+          message = hipchat.send(:create_message, data)
 
-        expect(message).to eq("#{user.name} commented on " \
-            "<a href=\"#{obj_attr[:url]}\">issue ##{issue_id}</a> in " \
-            "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
-            "<b>#{title}</b>" \
-            "<pre>issue note</pre>")
+          obj_attr = data[:object_attributes]
+          issue_id = data[:issue]['iid']
+          title = data[:issue]['title']
+
+          expect(message).to eq("#{user.name} commented on " \
+              "<a href=\"#{obj_attr[:url]}\">issue ##{issue_id}</a> in " \
+              "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
+              "<b>#{title}</b>" \
+              "<pre>issue note</pre>")
+        end
       end
 
-      it "should call Hipchat API for snippet comment events" do
-        data = Gitlab::NoteDataBuilder.build(snippet_note, user)
-        hipchat.execute(data)
+      context 'when snippet comment event triggered' do
+        let(:snippet) { create(:project_snippet, project: project) }
+        let(:snippet_note) do
+          create(:note_on_project_snippet, noteable_id: snippet.id,
+                                           project: project,
+                                           note: "snippet note")
+        end
 
-        expect(WebMock).to have_requested(:post, api_url).once
+        it "should call Hipchat API for snippet comment events" do
+          data = Gitlab::NoteDataBuilder.build(snippet_note, user)
+          hipchat.execute(data)
 
-        message = hipchat.send(:create_message, data)
+          expect(WebMock).to have_requested(:post, api_url).once
 
-        obj_attr = data[:object_attributes]
-        snippet_id = data[:snippet]['id']
-        title = data[:snippet]['title']
+          message = hipchat.send(:create_message, data)
 
-        expect(message).to eq("#{user.name} commented on " \
-            "<a href=\"#{obj_attr[:url]}\">snippet ##{snippet_id}</a> in " \
-            "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
-            "<b>#{title}</b>" \
-            "<pre>snippet note</pre>")
+          obj_attr = data[:object_attributes]
+          snippet_id = data[:snippet]['id']
+          title = data[:snippet]['title']
+
+          expect(message).to eq("#{user.name} commented on " \
+              "<a href=\"#{obj_attr[:url]}\">snippet ##{snippet_id}</a> in " \
+              "<a href=\"#{project.web_url}\">#{project_name}</a>: " \
+              "<b>#{title}</b>" \
+              "<pre>snippet note</pre>")
+        end
       end
     end
 
diff --git a/spec/models/project_services/slack_service_spec.rb b/spec/models/project_services/slack_service_spec.rb
index a97b7560137..155f3e74e0d 100644
--- a/spec/models/project_services/slack_service_spec.rb
+++ b/spec/models/project_services/slack_service_spec.rb
@@ -142,13 +142,6 @@ describe SlackService, models: true do
     let(:slack)   { SlackService.new }
     let(:user) { create(:user) }
     let(:project) { create(:project, creator_id: user.id) }
-    let(:issue)         { create(:issue, project: project) }
-    let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
-    let(:snippet)       { create(:project_snippet, project: project) }
-    let(:commit_note) { create(:note_on_commit, author: user, project: project, commit_id: project.repository.commit.id, note: 'a comment on a commit') }
-    let(:merge_request_note) { create(:note_on_merge_request, noteable_id: merge_request.id, note: "merge request note") }
-    let(:issue_note) { create(:note_on_issue, noteable_id: issue.id, note: "issue note")}
-    let(:snippet_note) { create(:note_on_project_snippet, noteable_id: snippet.id, note: "snippet note") }
     let(:webhook_url) { 'https://hooks.slack.com/services/SVRWFV0VVAR97N/B02R25XN3/ZBqu7xMupaEEICInN685' }
 
     before do
@@ -162,32 +155,61 @@ describe SlackService, models: true do
       WebMock.stub_request(:post, webhook_url)
     end
 
-    it "should call Slack API for commit comment events" do
-      data = Gitlab::NoteDataBuilder.build(commit_note, user)
-      slack.execute(data)
+    context 'when commit comment event executed' do
+      let(:commit_note) do
+        create(:note_on_commit, author: user,
+                                project: project,
+                                commit_id: project.repository.commit.id,
+                                note: 'a comment on a commit')
+      end
 
-      expect(WebMock).to have_requested(:post, webhook_url).once
+      it "should call Slack API for commit comment events" do
+        data = Gitlab::NoteDataBuilder.build(commit_note, user)
+        slack.execute(data)
+
+        expect(WebMock).to have_requested(:post, webhook_url).once
+      end
     end
 
-    it "should call Slack API for merge request comment events" do
-      data = Gitlab::NoteDataBuilder.build(merge_request_note, user)
-      slack.execute(data)
+    context 'when merge request comment event executed' do
+      let(:merge_request_note) do
+        create(:note_on_merge_request, project: project,
+                                       note: "merge request note")
+      end
 
-      expect(WebMock).to have_requested(:post, webhook_url).once
+      it "should call Slack API for merge request comment events" do
+        data = Gitlab::NoteDataBuilder.build(merge_request_note, user)
+        slack.execute(data)
+
+        expect(WebMock).to have_requested(:post, webhook_url).once
+      end
     end
 
-    it "should call Slack API for issue comment events" do
-      data = Gitlab::NoteDataBuilder.build(issue_note, user)
-      slack.execute(data)
+    context 'when issue comment event executed' do
+      let(:issue_note) do
+        create(:note_on_issue, project: project, note: "issue note")
+      end
 
-      expect(WebMock).to have_requested(:post, webhook_url).once
+      it "should call Slack API for issue comment events" do
+        data = Gitlab::NoteDataBuilder.build(issue_note, user)
+        slack.execute(data)
+
+        expect(WebMock).to have_requested(:post, webhook_url).once
+      end
     end
 
-    it "should call Slack API for snippet comment events" do
-      data = Gitlab::NoteDataBuilder.build(snippet_note, user)
-      slack.execute(data)
+    context 'when snippet comment event executed' do
+      let(:snippet_note) do
+        create(:note_on_project_snippet, project: project,
+                                         note: "snippet note")
+      end
 
-      expect(WebMock).to have_requested(:post, webhook_url).once
+      it "should call Slack API for snippet comment events" do
+        data = Gitlab::NoteDataBuilder.build(snippet_note, user)
+        slack.execute(data)
+
+        expect(WebMock).to have_requested(:post, webhook_url).once
+      end
     end
   end
 end
diff --git a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb
index 82247849814..0861d74aede 100644
--- a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb
+++ b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb
@@ -1,8 +1,8 @@
 require 'spec_helper'
 
 describe MergeRequests::MergeWhenBuildSucceedsService do
-  let(:user)          { create(:user) }
-  let(:merge_request) { create(:merge_request) }
+  let(:user) { create(:user) }
+  let(:project) { create(:project) }
 
   let(:mr_merge_if_green_enabled) do
     create(:merge_request, merge_when_build_succeeds: true, merge_user: user,
@@ -10,11 +10,15 @@ describe MergeRequests::MergeWhenBuildSucceedsService do
                            source_project: project, target_project: project, state: "opened")
   end
 
-  let(:project) { create(:project) }
   let(:ci_commit) { create(:ci_commit_with_one_job, ref: mr_merge_if_green_enabled.source_branch, project: project) }
   let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, commit_message: 'Awesome message') }
 
   describe "#execute" do
+    let(:merge_request) do
+      create(:merge_request, target_project: project, source_project: project,
+                             source_branch: "feature", target_branch: 'master')
+    end
+
     context 'first time enabling' do
       before do
         allow(merge_request).to receive(:ci_commit).and_return(ci_commit)
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index ee976eb2926..29e0a63d8ce 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -208,8 +208,10 @@ describe SystemNoteService, services: true do
   end
 
   describe '.merge_when_build_succeeds' do
-    let(:ci_commit) { build :ci_commit_without_jobs }
-    let(:noteable) { create :merge_request }
+    let(:ci_commit) { build(:ci_commit_without_jobs )}
+    let(:noteable) do
+      create(:merge_request, source_project: project, target_project: project)
+    end
 
     subject { described_class.merge_when_build_succeeds(noteable, project, author, noteable.last_commit) }
 
@@ -221,8 +223,10 @@ describe SystemNoteService, services: true do
   end
 
   describe '.cancel_merge_when_build_succeeds' do
-    let(:ci_commit) { build :ci_commit_without_jobs }
-    let(:noteable) { create :merge_request }
+    let(:ci_commit) { build(:ci_commit_without_jobs) }
+    let(:noteable) do
+      create(:merge_request, source_project: project, target_project: project)
+    end
 
     subject { described_class.cancel_merge_when_build_succeeds(noteable, project, author) }
 

From 99ef3a84b558e7b51bce7cbb11a6e235a6cc310b Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 29 Apr 2016 12:16:18 +0200
Subject: [PATCH 10/16] Validate presence of noteable_type in note model

---
 app/models/note.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/models/note.rb b/app/models/note.rb
index e3e522a8d0f..5f669c02e8b 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -29,6 +29,7 @@ class Note < ActiveRecord::Base
   # Attachments are deprecated and are handled by Markdown uploader
   validates :attachment, file_size: { maximum: :max_attachment_size }
 
+  validates :noteable_type, presence: true
   validates :noteable_id, presence: true, unless: :for_commit?
   validates :commit_id, presence: true, if: :for_commit?
   validates :author, presence: true

From fc57d36018a23c15da013bebf42d51f7a8e9a955 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 4 May 2016 11:17:16 +0200
Subject: [PATCH 11/16] Minor changes in note validation specs

---
 spec/factories/notes.rb               | 22 ++++++++++++----------
 spec/models/concerns/issuable_spec.rb |  6 ++++--
 spec/models/merge_request_spec.rb     |  2 --
 spec/models/note_spec.rb              | 11 ++++++-----
 4 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index 2940ac342a2..6801438165d 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -9,9 +9,9 @@ FactoryGirl.define do
     author
     noteable { create(:issue, project: project) }
 
-    factory :note_on_issue,              traits: [:on_issue], aliases: [:votable_note]
     factory :note_on_commit,             traits: [:on_commit]
     factory :note_on_commit_diff,        traits: [:on_commit, :on_diff], class: LegacyDiffNote
+    factory :note_on_issue,              traits: [:on_issue], aliases: [:votable_note]
     factory :note_on_merge_request,      traits: [:on_merge_request]
     factory :note_on_merge_request_diff, traits: [:on_merge_request, :on_diff], class: LegacyDiffNote
     factory :note_on_project_snippet,    traits: [:on_project_snippet]
@@ -19,14 +19,20 @@ FactoryGirl.define do
     factory :downvote_note,              traits: [:award, :downvote]
     factory :upvote_note,                traits: [:award, :upvote]
 
-    trait :on_issue do
-      noteable_type 'Issue'
-    end
-
     trait :on_commit do
       noteable nil
+      noteable_type 'Commit'
+      noteable_id nil
       commit_id RepoHelpers.sample_commit.id
-      noteable_type "Commit"
+    end
+
+    trait :on_diff do
+      line_code "0_184_184"
+    end
+
+    trait :on_issue do
+      noteable_type 'Issue'
+      noteable { create(:issue, project: project) }
     end
 
     trait :on_merge_request do
@@ -42,10 +48,6 @@ FactoryGirl.define do
       noteable { create(:snippet, project: project) }
     end
 
-    trait :on_diff do
-      line_code "0_184_184"
-    end
-
     trait :system do
       system true
     end
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 331ebaf4ac4..70bbe633269 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -228,9 +228,11 @@ describe Issue, "Issuable" do
   end
 
   describe "votes" do
+    let(:project) { issue.project }
+
     before do
-      issue.notes.awards.create!(note: "thumbsup", author: user, project: issue.project)
-      issue.notes.awards.create!(note: "thumbsdown", author: user, project: issue.project)
+      issue.notes.awards.create!(note: "thumbsup", author: user, project: project)
+      issue.notes.awards.create!(note: "thumbsdown", author: user, project: project)
     end
 
     it "returns correct values" do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 20e383e03b4..4b67c2facf3 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -120,7 +120,6 @@ describe MergeRequest, models: true do
     before do
       allow(merge_request).to receive(:commits) { [merge_request.source_project.repository.commit] }
       create(:note_on_commit, commit_id: merge_request.commits.first.id,
-                              noteable_type: 'Commit',
                               project: merge_request.project)
       create(:note, noteable: merge_request, project: merge_request.project)
     end
@@ -132,7 +131,6 @@ describe MergeRequest, models: true do
 
     it "should include notes for commits from target project as well" do
       create(:note_on_commit, commit_id: merge_request.commits.first.id,
-                              noteable_type: 'Commit',
                               project: merge_request.target_project)
 
       expect(merge_request.commits).not_to be_empty
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 20d40c47aa6..e1b81b23d4b 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -13,29 +13,30 @@ describe Note, models: true do
     it { is_expected.to validate_presence_of(:note) }
     it { is_expected.to validate_presence_of(:project) }
 
-    context 'when note is comment on commit' do
+    context 'when note is on commit' do
       before { allow(subject).to receive(:for_commit?).and_return(true) }
 
       it { is_expected.to validate_presence_of(:commit_id) }
       it { is_expected.to_not validate_presence_of(:noteable_id) }
     end
 
-    context 'when note is not comment on commit' do
+    context 'when note is not on commit' do
       before { allow(subject).to receive(:for_commit?).and_return(false) }
 
       it { is_expected.to_not validate_presence_of(:commit_id) }
       it { is_expected.to validate_presence_of(:noteable_id) }
     end
 
-    context 'when noteable and note project is different' do
+    context 'when noteable and note project differ' do
       subject do
-        build(:note, noteable: create(:issue), project: create(:project))
+        build(:note, noteable: build_stubbed(:issue),
+                     project: build_stubbed(:project))
       end
 
       it { is_expected.to be_invalid }
     end
 
-    context 'when noteable and note project is the same one' do
+    context 'when noteable and note project are the same' do
       subject { create(:note) }
       it { is_expected.to be_valid }
     end

From 77648aac86bdea2cd023604552b73679baf755b2 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 6 May 2016 10:12:08 +0200
Subject: [PATCH 12/16] Add Changelog entry for note validation improvements

---
 CHANGELOG | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG b/CHANGELOG
index 5dc56d18c3a..82be2542954 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -63,6 +63,7 @@ v 8.8.0
   - Make 'upcoming' filter for milestones work better across projects
   - Sanitize repo paths in new project error message
   - Bump mail_room to 0.7.0 to fix stuck IDLE connections
+  - Improve note validation to prevent errors when creating invalid note via API
   - Remove future dates from contribution calendar graph.
   - Support e-mail notifications for comments on project snippets
   - Fix API leak of notes of unauthorized issues, snippets and merge requests

From cc2efcf1a681223d5c74eef6fca73cde71dce7c2 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 18 May 2016 18:21:14 +0200
Subject: [PATCH 13/16] Improve notes factory

---
 spec/factories/notes.rb | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index 6801438165d..6f9b3568e70 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -37,10 +37,7 @@ FactoryGirl.define do
 
     trait :on_merge_request do
       noteable_type 'MergeRequest'
-      noteable do
-        create(:merge_request, source_project: project,
-                               target_project: project)
-      end
+      noteable { create(:merge_request, source_project: project) }
     end
 
     trait :on_project_snippet do

From dbba60029c84fa091350d3a7d8b2e73cfec25f7e Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 18 May 2016 19:38:52 +0200
Subject: [PATCH 14/16] Improve note factory

---
 features/steps/project/merge_requests.rb         |  2 +-
 spec/factories/notes.rb                          |  7 ++-----
 spec/features/issues/note_polling_spec.rb        |  5 ++---
 spec/lib/gitlab/note_data_builder_spec.rb        | 16 ++++++++--------
 spec/models/legacy_diff_note_spec.rb             |  4 +++-
 .../project_services/hipchat_service_spec.rb     |  6 +++---
 6 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb
index cfa586f859f..b2fa8750234 100644
--- a/features/steps/project/merge_requests.rb
+++ b/features/steps/project/merge_requests.rb
@@ -273,7 +273,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
   step 'user "John Doe" leaves a comment like "Line is wrong" on diff' do
     mr = MergeRequest.find_by(title: "Bug NS-05")
     create(:note_on_merge_request_diff, project: project,
-                                        noteable_id: mr.id,
+                                        noteable: mr,
                                         author: user_exists("John Doe"),
                                         line_code: sample_commit.line_code,
                                         note: 'Line is wrong')
diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index 6f9b3568e70..c32e205ee69 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -7,7 +7,7 @@ FactoryGirl.define do
     project
     note "Note"
     author
-    noteable { create(:issue, project: project) }
+    on_issue
 
     factory :note_on_commit,             traits: [:on_commit]
     factory :note_on_commit_diff,        traits: [:on_commit, :on_diff], class: LegacyDiffNote
@@ -21,8 +21,8 @@ FactoryGirl.define do
 
     trait :on_commit do
       noteable nil
-      noteable_type 'Commit'
       noteable_id nil
+      noteable_type 'Commit'
       commit_id RepoHelpers.sample_commit.id
     end
 
@@ -31,17 +31,14 @@ FactoryGirl.define do
     end
 
     trait :on_issue do
-      noteable_type 'Issue'
       noteable { create(:issue, project: project) }
     end
 
     trait :on_merge_request do
-      noteable_type 'MergeRequest'
       noteable { create(:merge_request, source_project: project) }
     end
 
     trait :on_project_snippet do
-      noteable_type 'Snippet'
       noteable { create(:snippet, project: project) }
     end
 
diff --git a/spec/features/issues/note_polling_spec.rb b/spec/features/issues/note_polling_spec.rb
index 9f521263e4c..f5cfe2d666e 100644
--- a/spec/features/issues/note_polling_spec.rb
+++ b/spec/features/issues/note_polling_spec.rb
@@ -9,9 +9,8 @@ feature 'Issue notes polling' do
   end
 
   scenario 'Another user adds a comment to an issue', js: true do
-    note = create(:note_on_issue, noteable: issue,
-                                  project: project,
-                                  note: 'Looks good!')
+    note = create(:note, noteable: issue, project: project,
+                         note: 'Looks good!')
 
     page.execute_script('notes.refresh();')
 
diff --git a/spec/lib/gitlab/note_data_builder_spec.rb b/spec/lib/gitlab/note_data_builder_spec.rb
index 82823e7f790..e848d88182f 100644
--- a/spec/lib/gitlab/note_data_builder_spec.rb
+++ b/spec/lib/gitlab/note_data_builder_spec.rb
@@ -44,13 +44,13 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
     end
 
     let(:note) do
-      create(:note_on_issue, noteable_id: issue.id, project: project)
+      create(:note_on_issue, noteable: issue, project: project)
     end
 
     it 'returns the note and issue-specific data' do
       expect(data).to have_key(:issue)
       expect(data[:issue].except('updated_at'))
-        .to eq(issue.hook_attrs.except('updated_at'))
+        .to eq(issue.reload.hook_attrs.except('updated_at'))
       expect(data[:issue]['updated_at'])
         .to be > issue.hook_attrs['updated_at']
     end
@@ -67,14 +67,14 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
     end
 
     let(:note) do
-      create(:note_on_merge_request, noteable_id: merge_request.id,
+      create(:note_on_merge_request, noteable: merge_request,
                                      project: project)
     end
 
     it 'returns the note and merge request data' do
       expect(data).to have_key(:merge_request)
       expect(data[:merge_request].except('updated_at'))
-        .to eq(merge_request.hook_attrs.except('updated_at'))
+        .to eq(merge_request.reload.hook_attrs.except('updated_at'))
       expect(data[:merge_request]['updated_at'])
         .to be > merge_request.hook_attrs['updated_at']
     end
@@ -90,14 +90,14 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
     end
 
     let(:note) do
-      create(:note_on_merge_request_diff, noteable_id: merge_request.id,
+      create(:note_on_merge_request_diff, noteable: merge_request,
                                           project: project)
     end
 
     it 'returns the note and merge request diff data' do
       expect(data).to have_key(:merge_request)
       expect(data[:merge_request].except('updated_at'))
-        .to eq(merge_request.hook_attrs.except('updated_at'))
+        .to eq(merge_request.reload.hook_attrs.except('updated_at'))
       expect(data[:merge_request]['updated_at'])
         .to be > merge_request.hook_attrs['updated_at']
     end
@@ -113,14 +113,14 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
     end
 
     let!(:note) do
-      create(:note_on_project_snippet, noteable_id: snippet.id,
+      create(:note_on_project_snippet, noteable: snippet,
                                        project: project)
     end
 
     it 'returns the note and project snippet data' do
       expect(data).to have_key(:snippet)
       expect(data[:snippet].except('updated_at'))
-        .to eq(snippet.hook_attrs.except('updated_at'))
+        .to eq(snippet.reload.hook_attrs.except('updated_at'))
       expect(data[:snippet]['updated_at'])
         .to be > snippet.hook_attrs['updated_at']
     end
diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb
index 7c29bef54e4..b2d06853886 100644
--- a/spec/models/legacy_diff_note_spec.rb
+++ b/spec/models/legacy_diff_note_spec.rb
@@ -63,7 +63,9 @@ describe LegacyDiffNote, models: true do
         code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos)
 
         # We're persisting in order to trigger the set_diff callback
-        note = create(:note_on_merge_request_diff, noteable: merge, line_code: code)
+        note = create(:note_on_merge_request_diff, noteable: merge,
+                                                   line_code: code,
+                                                   project: merge.source_project)
 
         # Make sure we don't get a false positive from a guard clause
         expect(note).to receive(:find_noteable_diff).and_call_original
diff --git a/spec/models/project_services/hipchat_service_spec.rb b/spec/models/project_services/hipchat_service_spec.rb
index 7013ffbf3e1..5f618322aab 100644
--- a/spec/models/project_services/hipchat_service_spec.rb
+++ b/spec/models/project_services/hipchat_service_spec.rb
@@ -211,7 +211,7 @@ describe HipchatService, models: true do
         end
 
         let(:merge_request_note) do
-          create(:note_on_merge_request, noteable_id: merge_request.id,
+          create(:note_on_merge_request, noteable: merge_request,
                                          project: project,
                                          note: "merge request note")
         end
@@ -239,7 +239,7 @@ describe HipchatService, models: true do
       context 'when issue comment event triggered' do
         let(:issue) { create(:issue, project: project) }
         let(:issue_note) do
-          create(:note_on_issue, noteable_id: issue.id, project: project,
+          create(:note_on_issue, noteable: issue, project: project,
                                  note: "issue note")
         end
 
@@ -264,7 +264,7 @@ describe HipchatService, models: true do
       context 'when snippet comment event triggered' do
         let(:snippet) { create(:project_snippet, project: project) }
         let(:snippet_note) do
-          create(:note_on_project_snippet, noteable_id: snippet.id,
+          create(:note_on_project_snippet, noteable: snippet,
                                            project: project,
                                            note: "snippet note")
         end

From 1286b9c3b5bc2435406e94127f8036d032623f7e Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 24 May 2016 02:34:44 +0200
Subject: [PATCH 15/16] Move Changelog entry for note validation to 8.9

---
 CHANGELOG | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CHANGELOG b/CHANGELOG
index 82be2542954..160543d8bae 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.9.0 (unreleased)
   - Allow forking projects with restricted visibility level
+  - Improve note validation to prevent errors when creating invalid note via API
   - Redesign navigation for project pages
   - Fix groups API to list only user's accessible projects
   - Redesign account and email confirmation emails
@@ -63,7 +64,6 @@ v 8.8.0
   - Make 'upcoming' filter for milestones work better across projects
   - Sanitize repo paths in new project error message
   - Bump mail_room to 0.7.0 to fix stuck IDLE connections
-  - Improve note validation to prevent errors when creating invalid note via API
   - Remove future dates from contribution calendar graph.
   - Support e-mail notifications for comments on project snippets
   - Fix API leak of notes of unauthorized issues, snippets and merge requests

From f36e7ff2f9c8eef4c5ca60fac0aa32361bc5374f Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Sun, 29 May 2016 23:12:19 -0400
Subject: [PATCH 16/16] Shut up, RuboCop

:heart:
---
 spec/models/note_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index e1b81b23d4b..8aad1b73add 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -17,13 +17,13 @@ describe Note, models: true do
       before { allow(subject).to receive(:for_commit?).and_return(true) }
 
       it { is_expected.to validate_presence_of(:commit_id) }
-      it { is_expected.to_not validate_presence_of(:noteable_id) }
+      it { is_expected.not_to validate_presence_of(:noteable_id) }
     end
 
     context 'when note is not on commit' do
       before { allow(subject).to receive(:for_commit?).and_return(false) }
 
-      it { is_expected.to_not validate_presence_of(:commit_id) }
+      it { is_expected.not_to validate_presence_of(:commit_id) }
       it { is_expected.to validate_presence_of(:noteable_id) }
     end