diff --git a/CHANGELOG b/CHANGELOG
index 5dc56d18c3a01ca30de4c2e039b617b7166ca694..160543d8bae93e1d074825a2758abe56ccd85f33 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
diff --git a/app/models/note.rb b/app/models/note.rb
index 55b98557244aaab7fdc5534f01c60bdb09b57ed6..5f669c02e8ba155c5fcd9176edec0350089ed582 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -29,10 +29,17 @@ 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_type, presence: true
+ validates :noteable_id, presence: true, unless: :for_commit?
+ validates :commit_id, presence: true, if: :for_commit?
validates :author, presence: true
+ validate unless: :for_commit? do |note|
+ unless note.noteable.try(:project) == note.project
+ errors.add(:invalid_project, 'Note and noteable project mismatch')
+ end
+ end
+
mount_uploader :attachment, AttachmentUploader
# Scopes
diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index 01586994813cca5dcca3fe8f628c058d256425dd..2bb312bb252ca4096d0b9c22180bca22fcea42e5 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -5,8 +5,6 @@ def execute
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 @@ def execute
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
diff --git a/features/steps/dashboard/todos.rb b/features/steps/dashboard/todos.rb
index d3b6c7f6a15c6b25901a5b7153ce1e7021f131a8..bd8a270202ee750368e12ad4fef0e73908982257 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 f2c68f007ef0d3d79e52500f97b509c3c3747d0f..5cd431e05d58025f8a7a92ac7bc106b212921a66 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/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb
index cfa586f859f6832d03fbf87e6f7f042efb42e5e5..b2fa8750234fd44858ab2407e6eb20c092811539 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 26719f2652c277f2ab4b7c0fff0b0a957a62d2bc..c32e205ee69bb2e7b0d95361da43a7c5ee23bbf4 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -7,6 +7,7 @@
project
note "Note"
author
+ on_issue
factory :note_on_commit, traits: [:on_commit]
factory :note_on_commit_diff, traits: [:on_commit, :on_diff], class: LegacyDiffNote
@@ -19,29 +20,26 @@
factory :upvote_note, traits: [:award, :upvote]
trait :on_commit do
- project
+ noteable nil
+ noteable_id nil
+ noteable_type 'Commit'
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
- project
- noteable_id 1
- noteable_type "MergeRequest"
+ trait :on_issue do
+ noteable { create(:issue, project: project) }
end
- trait :on_issue do
- noteable_id 1
- noteable_type "Issue"
+ trait :on_merge_request do
+ noteable { create(:merge_request, source_project: project) }
end
trait :on_project_snippet do
- noteable_id 1
- noteable_type "Snippet"
+ noteable { create(:snippet, project: project) }
end
trait :system do
diff --git a/spec/features/issues/note_polling_spec.rb b/spec/features/issues/note_polling_spec.rb
index e4efdbe2421d0dfc5838dcba1bac30cb705c4e2b..f5cfe2d666ef93e3b1bc3f8a3887cfae7d621256 100644
--- a/spec/features/issues/note_polling_spec.rb
+++ b/spec/features/issues/note_polling_spec.rb
@@ -9,8 +9,11 @@
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, 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 749ee01890c9e234dd5a7d1ff5aec4ee3535a4b8..9271964166a7dd8bc2a4b403f3a16ee3f8ef4c0d 100644
--- a/spec/features/issues_spec.rb
+++ b/spec/features/issues_spec.rb
@@ -125,7 +125,7 @@
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 9e9fec01943368eff2ed82ca8473b50bf01110d5..2835cf444949202088e2d14eda353014f6749b47 100644
--- a/spec/features/notes_on_merge_requests_spec.rb
+++ b/spec/features/notes_on_merge_requests_spec.rb
@@ -19,10 +19,14 @@
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 1adab7e9c6c3ef66f70eed0ecf83d0f081b8359b..c7c00a3266a32cfe5bd3e84301e7861b75d1e64f 100644
--- a/spec/features/participants_autocomplete_spec.rb
+++ b/spec/features/participants_autocomplete_spec.rb
@@ -32,7 +32,8 @@
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 @@
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 b7368cca29d0cdb3dbfcca4f7910718866e10cc7..6ed279ef9be6f3413b683b6c793e72cf7ca6e2ad 100644
--- a/spec/features/task_lists_spec.rb
+++ b/spec/features/task_lists_spec.rb
@@ -75,7 +75,10 @@ def visit_issue(project, issue)
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 f093d0a0d8b3a7dd5b481841cf7e92d68abdc783..e848d88182fb07b030684e283ce80211d7563cd4 100644
--- a/spec/lib/gitlab/note_data_builder_spec.rb
+++ b/spec/lib/gitlab/note_data_builder_spec.rb
@@ -9,7 +9,8 @@
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 @@
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: 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'))
- expect(data[:issue]['updated_at']).to be > issue.hook_attrs['updated_at']
+ expect(data[:issue].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
include_examples 'project hook data'
@@ -51,13 +60,23 @@
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: 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'))
- 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.reload.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 @@
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: 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'))
- 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.reload.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 @@
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: 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'))
- expect(data[:snippet]['updated_at']).to be > snippet.hook_attrs['updated_at']
+ expect(data[:snippet].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
include_examples 'project hook data'
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 583827cdf42fb93faae9a00f37ad35f495ed31bc..70bbe63326930ac52f2c842219d14bc7c64e6c6d 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -189,7 +189,6 @@
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)
@@ -229,11 +228,11 @@
end
describe "votes" do
+ let(:project) { issue.project }
+
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: project)
+ issue.notes.awards.create!(note: "thumbsdown", author: user, project: project)
end
it "returns correct values" do
diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb
index 7c29bef54e48b8993c02d7f32e04eaccfeceddc2..b2d068538864d05f6808c51724fc243950b96ba2 100644
--- a/spec/models/legacy_diff_note_spec.rb
+++ b/spec/models/legacy_diff_note_spec.rb
@@ -63,7 +63,9 @@
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/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index e269ff26a047f40f9d5f60c648edaf8a3cb773b5..4b67c2facf32a7e7c0476f4d32b9d1d767b98187 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -119,7 +119,8 @@
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,
+ project: merge_request.project)
create(:note, noteable: merge_request, project: merge_request.project)
end
@@ -129,7 +130,9 @@
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,
+ 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 5d916f0e6a65b2263cb506f583e033438d2f7436..8aad1b73add203d4826fec1204b83b561db223c3 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -12,6 +12,34 @@
describe 'validation' do
it { is_expected.to validate_presence_of(:note) }
it { is_expected.to validate_presence_of(:project) }
+
+ 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.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.not_to validate_presence_of(:commit_id) }
+ it { is_expected.to validate_presence_of(:noteable_id) }
+ end
+
+ context 'when noteable and note project differ' do
+ subject do
+ build(:note, noteable: build_stubbed(:issue),
+ project: build_stubbed(:project))
+ end
+
+ it { is_expected.to be_invalid }
+ end
+
+ context 'when noteable and note project are the same' do
+ subject { create(:note) }
+ it { is_expected.to be_valid }
+ end
end
describe "Commit notes" do
@@ -89,8 +117,8 @@
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 +130,7 @@
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])
@@ -175,12 +203,18 @@
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 f887ed4bafc14493a63520e20ddd49815da761cb..5f618322aabe07df6128cb6acc05c79d0f93e4f8 100644
--- a/spec/models/project_services/hipchat_service_spec.rb
+++ b/spec/models/project_services/hipchat_service_spec.rb
@@ -176,86 +176,117 @@
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)
- expect(WebMock).to have_requested(:post, api_url).once
+ 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
+
+ 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: merge_request,
+ 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: issue, 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: snippet,
+ 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 a97b7560137fdaec9860916a2926690bf2b71341..155f3e74e0d24989c51bdb357f494927477d481a 100644
--- a/spec/models/project_services/slack_service_spec.rb
+++ b/spec/models/project_services/slack_service_spec.rb
@@ -142,13 +142,6 @@
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 @@
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/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb
index ed1ed5aeb954a18a92030e4c9f5e404488d4f0c4..beb29a686920987541bb3b3f24c0b0d49934ba71 100644
--- a/spec/requests/api/notes_spec.rb
+++ b/spec/requests/api/notes_spec.rb
@@ -258,8 +258,8 @@
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
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 8224784981448d216667cb23b4ab6f57ebb8758a..0861d74aeded4729fdc9a803aa38a16ac6e114fb 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 @@
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 ee976eb2926caddee28e52ce2521f892d609a059..29e0a63d8ce1d700bc39d592fc150e9861063b3a 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -208,8 +208,10 @@
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 @@
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) }