Commit 43c35b0f authored by Robert Speicher's avatar Robert Speicher

Merge branch 'feature/note-validator' into 'master'

Improve note validation

This MR improves note validation. Originates from #15577.

Closes #17260

See merge request !4024
parents c84ec18f f36e7ff2
...@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date. ...@@ -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) v 8.9.0 (unreleased)
- Allow forking projects with restricted visibility level - Allow forking projects with restricted visibility level
- Improve note validation to prevent errors when creating invalid note via API
- Redesign navigation for project pages - Redesign navigation for project pages
- Fix groups API to list only user's accessible projects - Fix groups API to list only user's accessible projects
- Redesign account and email confirmation emails - Redesign account and email confirmation emails
......
...@@ -29,10 +29,17 @@ class Note < ActiveRecord::Base ...@@ -29,10 +29,17 @@ class Note < ActiveRecord::Base
# Attachments are deprecated and are handled by Markdown uploader # Attachments are deprecated and are handled by Markdown uploader
validates :attachment, file_size: { maximum: :max_attachment_size } validates :attachment, file_size: { maximum: :max_attachment_size }
validates :noteable_id, presence: true, if: ->(n) { n.noteable_type.present? && n.noteable_type != 'Commit' } validates :noteable_type, presence: true
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 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 mount_uploader :attachment, AttachmentUploader
# Scopes # Scopes
......
...@@ -5,8 +5,6 @@ module Notes ...@@ -5,8 +5,6 @@ module Notes
note.author = current_user note.author = current_user
note.system = false note.system = false
return unless valid_project?(note)
if note.save if note.save
# Finish the harder work in the background # Finish the harder work in the background
NewNoteWorker.perform_in(2.seconds, note.id, params) NewNoteWorker.perform_in(2.seconds, note.id, params)
...@@ -15,14 +13,5 @@ module Notes ...@@ -15,14 +13,5 @@ module Notes
note note
end end
private
def valid_project?(note)
return false unless project
return true if note.for_commit?
note.noteable.try(:project) == project
end
end end
end end
...@@ -20,7 +20,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps ...@@ -20,7 +20,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
step 'I have todos' do 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: mary_jane, target: issue, action: Todo::MENTIONED)
create(:todo, user: current_user, project: project, author: john_doe, target: issue, action: Todo::ASSIGNED) 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: issue, action: Todo::MENTIONED, note: note)
create(:todo, user: current_user, project: project, author: john_doe, target: merge_request, action: Todo::ASSIGNED) create(:todo, user: current_user, project: project, author: john_doe, target: merge_request, action: Todo::ASSIGNED)
end end
......
...@@ -348,7 +348,7 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps ...@@ -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 step 'another user adds a comment with text "Yay!" to issue "Release 0.4"' do
issue = Issue.find_by!(title: 'Release 0.4') 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 end
step 'I should see a new comment with text "Yay!"' do step 'I should see a new comment with text "Yay!"' do
......
...@@ -273,7 +273,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps ...@@ -273,7 +273,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
step 'user "John Doe" leaves a comment like "Line is wrong" on diff' do step 'user "John Doe" leaves a comment like "Line is wrong" on diff' do
mr = MergeRequest.find_by(title: "Bug NS-05") mr = MergeRequest.find_by(title: "Bug NS-05")
create(:note_on_merge_request_diff, project: project, create(:note_on_merge_request_diff, project: project,
noteable_id: mr.id, noteable: mr,
author: user_exists("John Doe"), author: user_exists("John Doe"),
line_code: sample_commit.line_code, line_code: sample_commit.line_code,
note: 'Line is wrong') note: 'Line is wrong')
......
...@@ -7,6 +7,7 @@ FactoryGirl.define do ...@@ -7,6 +7,7 @@ FactoryGirl.define do
project project
note "Note" note "Note"
author author
on_issue
factory :note_on_commit, traits: [:on_commit] factory :note_on_commit, traits: [:on_commit]
factory :note_on_commit_diff, traits: [:on_commit, :on_diff], class: LegacyDiffNote factory :note_on_commit_diff, traits: [:on_commit, :on_diff], class: LegacyDiffNote
...@@ -19,29 +20,26 @@ FactoryGirl.define do ...@@ -19,29 +20,26 @@ FactoryGirl.define do
factory :upvote_note, traits: [:award, :upvote] factory :upvote_note, traits: [:award, :upvote]
trait :on_commit do trait :on_commit do
project noteable nil
noteable_id nil
noteable_type 'Commit'
commit_id RepoHelpers.sample_commit.id commit_id RepoHelpers.sample_commit.id
noteable_type "Commit"
end end
trait :on_diff do trait :on_diff do
line_code "0_184_184" line_code "0_184_184"
end end
trait :on_merge_request do trait :on_issue do
project noteable { create(:issue, project: project) }
noteable_id 1
noteable_type "MergeRequest"
end end
trait :on_issue do trait :on_merge_request do
noteable_id 1 noteable { create(:merge_request, source_project: project) }
noteable_type "Issue"
end end
trait :on_project_snippet do trait :on_project_snippet do
noteable_id 1 noteable { create(:snippet, project: project) }
noteable_type "Snippet"
end end
trait :system do trait :system do
......
...@@ -9,8 +9,11 @@ feature 'Issue notes polling' do ...@@ -9,8 +9,11 @@ feature 'Issue notes polling' do
end end
scenario 'Another user adds a comment to an issue', js: true do 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();') page.execute_script('notes.refresh();')
expect(page).to have_selector("#note_#{note.id}", text: 'Looks good!') expect(page).to have_selector("#note_#{note.id}", text: 'Looks good!')
end end
end end
...@@ -125,7 +125,7 @@ describe 'Issues', feature: true do ...@@ -125,7 +125,7 @@ describe 'Issues', feature: true do
describe 'Issue info' do describe 'Issue info' do
it 'excludes award_emoji from comment count' do it 'excludes award_emoji from comment count' do
issue = create(:issue, author: @user, assignee: @user, project: project, title: 'foobar') 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) visit namespace_project_issues_path(project.namespace, project, assignee_id: @user.id)
......
...@@ -19,10 +19,14 @@ describe 'Comments', feature: true do ...@@ -19,10 +19,14 @@ describe 'Comments', feature: true do
end end
describe 'On a merge request', js: true, feature: true do describe 'On a merge request', js: true, feature: true do
let!(:merge_request) { create(:merge_request) } let!(:project) { create(:project) }
let!(:project) { merge_request.source_project } let!(:merge_request) do
create(:merge_request, source_project: project, target_project: project)
end
let!(:note) do 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 end
before do before do
......
...@@ -32,7 +32,8 @@ feature 'Member autocomplete', feature: true do ...@@ -32,7 +32,8 @@ feature 'Member autocomplete', feature: true do
context 'adding a new note on a Issue', js: true do context 'adding a new note on a Issue', js: true do
before do before do
issue = create(:issue, author: author, project: project) 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) visit_issue(project, issue)
end end
...@@ -47,7 +48,8 @@ feature 'Member autocomplete', feature: true do ...@@ -47,7 +48,8 @@ feature 'Member autocomplete', feature: true do
context 'adding a new note on a Merge Request ', js: true do context 'adding a new note on a Merge Request ', js: true do
before do before do
merge = create(:merge_request, source_project: project, target_project: project, author: author) 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) visit_merge_request(project, merge)
end end
......
...@@ -75,7 +75,10 @@ feature 'Task Lists', feature: true do ...@@ -75,7 +75,10 @@ feature 'Task Lists', feature: true do
describe 'for Notes' do describe 'for Notes' do
let!(:issue) { create(:issue, author: user, project: project) } 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 it 'renders for note body' do
visit_issue(project, issue) visit_issue(project, issue)
......
...@@ -9,7 +9,8 @@ describe 'Gitlab::NoteDataBuilder', lib: true do ...@@ -9,7 +9,8 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
before(:each) do before(:each) do
expect(data).to have_key(:object_attributes) expect(data).to have_key(:object_attributes)
expect(data[:object_attributes]).to have_key(:url) 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[:object_kind]).to eq('note')
expect(data[:user]).to eq(user.hook_attrs) expect(data[:user]).to eq(user.hook_attrs)
end end
...@@ -37,13 +38,21 @@ describe 'Gitlab::NoteDataBuilder', lib: true do ...@@ -37,13 +38,21 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
end end
describe 'When asking for a note on issue' do describe 'When asking for a note on issue' do
let(:issue) { create(:issue, created_at: fixed_time, updated_at: fixed_time) } let(:issue) do
let(:note) { create(:note_on_issue, noteable_id: issue.id, project: project) } 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 it 'returns the note and issue-specific data' do
expect(data).to have_key(:issue) expect(data).to have_key(:issue)
expect(data[:issue].except('updated_at')).to eq(issue.hook_attrs.except('updated_at')) expect(data[:issue].except('updated_at'))
expect(data[:issue]['updated_at']).to be > issue.hook_attrs['updated_at'] .to eq(issue.reload.hook_attrs.except('updated_at'))
expect(data[:issue]['updated_at'])
.to be > issue.hook_attrs['updated_at']
end end
include_examples 'project hook data' include_examples 'project hook data'
...@@ -51,13 +60,23 @@ describe 'Gitlab::NoteDataBuilder', lib: true do ...@@ -51,13 +60,23 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
end end
describe 'When asking for a note on merge request' do 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(:merge_request) do
let(:note) { create(:note_on_merge_request, noteable_id: merge_request.id, project: project) } 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 it 'returns the note and merge request data' do
expect(data).to have_key(:merge_request) 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].except('updated_at'))
expect(data[:merge_request]['updated_at']).to be > merge_request.hook_attrs['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 end
include_examples 'project hook data' include_examples 'project hook data'
...@@ -65,13 +84,22 @@ describe 'Gitlab::NoteDataBuilder', lib: true do ...@@ -65,13 +84,22 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
end end
describe 'When asking for a note on merge request diff' do 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(:merge_request) do
let(:note) { create(:note_on_merge_request_diff, noteable_id: merge_request.id, project: project) } 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 it 'returns the note and merge request diff data' do
expect(data).to have_key(:merge_request) 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].except('updated_at'))
expect(data[:merge_request]['updated_at']).to be > merge_request.hook_attrs['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 end
include_examples 'project hook data' include_examples 'project hook data'
...@@ -79,13 +107,22 @@ describe 'Gitlab::NoteDataBuilder', lib: true do ...@@ -79,13 +107,22 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
end end
describe 'When asking for a note on project snippet' do describe 'When asking for a note on project snippet' do
let!(:snippet) { create(:project_snippet, created_at: fixed_time, updated_at: fixed_time) } let!(:snippet) do
let!(:note) { create(:note_on_project_snippet, noteable_id: snippet.id, project: project) } 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 it 'returns the note and project snippet data' do
expect(data).to have_key(:snippet) expect(data).to have_key(:snippet)
expect(data[:snippet].except('updated_at')).to eq(snippet.hook_attrs.except('updated_at')) expect(data[:snippet].except('updated_at'))
expect(data[:snippet]['updated_at']).to be > snippet.hook_attrs['updated_at'] .to eq(snippet.reload.hook_attrs.except('updated_at'))
expect(data[:snippet]['updated_at'])
.to be > snippet.hook_attrs['updated_at']
end end
include_examples 'project hook data' include_examples 'project hook data'
......
...@@ -189,7 +189,6 @@ describe Issue, "Issuable" do ...@@ -189,7 +189,6 @@ describe Issue, "Issuable" do
let(:data) { issue.to_hook_data(user) } let(:data) { issue.to_hook_data(user) }
let(:project) { issue.project } let(:project) { issue.project }
it "returns correct hook data" do it "returns correct hook data" do
expect(data[:object_kind]).to eq("issue") expect(data[:object_kind]).to eq("issue")
expect(data[:user]).to eq(user.hook_attrs) expect(data[:user]).to eq(user.hook_attrs)
...@@ -229,11 +228,11 @@ describe Issue, "Issuable" do ...@@ -229,11 +228,11 @@ describe Issue, "Issuable" do
end end
describe "votes" do describe "votes" do
let(:project) { issue.project }
before do before do
author = create :user issue.notes.awards.create!(note: "thumbsup", author: user, project: project)
project = create :empty_project issue.notes.awards.create!(note: "thumbsdown", author: user, project: project)
issue.notes.awards.create!(note: "thumbsup", author: author, project: project)
issue.notes.awards.create!(note: "thumbsdown", author: author, project: project)
end end
it "returns correct values" do it "returns correct values" do
......
...@@ -63,7 +63,9 @@ describe LegacyDiffNote, models: true do ...@@ -63,7 +63,9 @@ describe LegacyDiffNote, models: true do
code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos) 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 # 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 # Make sure we don't get a false positive from a guard clause
expect(note).to receive(:find_noteable_diff).and_call_original expect(note).to receive(:find_noteable_diff).and_call_original
......
...@@ -119,7 +119,8 @@ describe MergeRequest, models: true do ...@@ -119,7 +119,8 @@ describe MergeRequest, models: true do
before do before do
allow(merge_request).to receive(:commits) { [merge_request.source_project.repository.commit] } 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) create(:note, noteable: merge_request, project: merge_request.project)
end end
...@@ -129,7 +130,9 @@ describe MergeRequest, models: true do ...@@ -129,7 +130,9 @@ describe MergeRequest, models: true do
end end
it "should include notes for commits from target project as well" do 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.commits).not_to be_empty
expect(merge_request.mr_and_commit_notes.count).to eq(3) expect(merge_request.mr_and_commit_notes.count).to eq(3)
end end
......
...@@ -12,6 +12,34 @@ describe Note, models: true do ...@@ -12,6 +12,34 @@ describe Note, models: true do
describe 'validation' do describe 'validation' do
it { is_expected.to validate_presence_of(:note) } it { is_expected.to validate_presence_of(:note) }
it { is_expected.to validate_presence_of(:project) } 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 end
describe "Commit notes" do describe "Commit notes" do
...@@ -89,8 +117,8 @@ describe Note, models: true do ...@@ -89,8 +117,8 @@ describe Note, models: true do
end end
describe "#all_references" do describe "#all_references" do
let!(:note1) { create(:note) } let!(:note1) { create(:note_on_issue) }
let!(:note2) { create(:note) } let!(:note2) { create(:note_on_issue) }
it "reads the rendered note body from the cache" do 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) expect(Banzai::Renderer).to receive(:render).with(note1.note, pipeline: :note, cache_key: [note1, "note"], project: note1.project)
...@@ -102,7 +130,7 @@ describe Note, models: true do ...@@ -102,7 +130,7 @@ describe Note, models: true do
end end
describe '.search' do describe '.search' do
let(:note) { create(:note, note: 'WoW') } let(:note) { create(:note_on_issue, note: 'WoW') }
it 'returns notes with matching content' do it 'returns notes with matching content' do
expect(described_class.search(note.note)).to eq([note]) expect(described_class.search(note.note)).to eq([note])
...@@ -175,12 +203,18 @@ describe Note, models: true do ...@@ -175,12 +203,18 @@ describe Note, models: true do
let(:merge_request) { create :merge_request } let(:merge_request) { create :merge_request }
it "converts aliases to actual name" do 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") expect(note.reload.note).to eq("thumbsup")
end end
it "is not an award emoji when comment is on a diff" do 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 note = note.reload
expect(note.note).to eq(":blowfish:") expect(note.note).to eq(":blowfish:")
......
...@@ -176,13 +176,13 @@ describe HipchatService, models: true do ...@@ -176,13 +176,13 @@ describe HipchatService, models: true do
context "Note events" do context "Note events" do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project, creator_id: user.id) } 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) } context 'when commit comment event triggered' do
let(:snippet) { create(:project_snippet, project: project) } let(:commit_note) do
let(:commit_note) { create(:note_on_commit, author: user, project: project, commit_id: project.repository.commit.id, note: 'a comment on a commit') } create(:note_on_commit, author: user, project: project,
let(:merge_request_note) { create(:note_on_merge_request, noteable_id: merge_request.id, note: "merge request note") } commit_id: project.repository.commit.id,
let(:issue_note) { create(:note_on_issue, noteable_id: issue.id, note: "issue note")} note: 'a comment on a commit')
let(:snippet_note) { create(:note_on_project_snippet, noteable_id: snippet.id, note: "snippet note") } end
it "should call Hipchat API for commit comment events" do it "should call Hipchat API for commit comment events" do
data = Gitlab::NoteDataBuilder.build(commit_note, user) data = Gitlab::NoteDataBuilder.build(commit_note, user)
...@@ -202,6 +202,19 @@ describe HipchatService, models: true do ...@@ -202,6 +202,19 @@ describe HipchatService, models: true do
"#{title}" \ "#{title}" \
"<pre>a comment on a commit</pre>") "<pre>a comment on a commit</pre>")
end end
end
context 'when merge request comment event triggered' do
let(:merge_request) do
create(:merge_request, source_project: project,
target_project: project)
end
let(:merge_request_note) do
create(:note_on_merge_request, noteable: merge_request,
project: project,
note: "merge request note")
end
it "should call Hipchat API for merge request comment events" do it "should call Hipchat API for merge request comment events" do
data = Gitlab::NoteDataBuilder.build(merge_request_note, user) data = Gitlab::NoteDataBuilder.build(merge_request_note, user)
...@@ -221,6 +234,14 @@ describe HipchatService, models: true do ...@@ -221,6 +234,14 @@ describe HipchatService, models: true do
"<b>#{title}</b>" \ "<b>#{title}</b>" \
"<pre>merge request note</pre>") "<pre>merge request note</pre>")
end end
end
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
it "should call Hipchat API for issue comment events" do it "should call Hipchat API for issue comment events" do
data = Gitlab::NoteDataBuilder.build(issue_note, user) data = Gitlab::NoteDataBuilder.build(issue_note, user)
...@@ -238,6 +259,15 @@ describe HipchatService, models: true do ...@@ -238,6 +259,15 @@ describe HipchatService, models: true do
"<b>#{title}</b>" \ "<b>#{title}</b>" \
"<pre>issue note</pre>") "<pre>issue note</pre>")
end end
end
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
it "should call Hipchat API for snippet comment events" do it "should call Hipchat API for snippet comment events" do
data = Gitlab::NoteDataBuilder.build(snippet_note, user) data = Gitlab::NoteDataBuilder.build(snippet_note, user)
...@@ -258,6 +288,7 @@ describe HipchatService, models: true do ...@@ -258,6 +288,7 @@ describe HipchatService, models: true do
"<pre>snippet note</pre>") "<pre>snippet note</pre>")
end end
end end
end
context 'build events' do context 'build events' do
let(:build) { create(:ci_build) } let(:build) { create(:ci_build) }
......
...@@ -142,13 +142,6 @@ describe SlackService, models: true do ...@@ -142,13 +142,6 @@ describe SlackService, models: true do
let(:slack) { SlackService.new } let(:slack) { SlackService.new }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project, creator_id: user.id) } 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' } let(:webhook_url) { 'https://hooks.slack.com/services/SVRWFV0VVAR97N/B02R25XN3/ZBqu7xMupaEEICInN685' }
before do before do
...@@ -162,12 +155,27 @@ describe SlackService, models: true do ...@@ -162,12 +155,27 @@ describe SlackService, models: true do
WebMock.stub_request(:post, webhook_url) WebMock.stub_request(:post, webhook_url)
end end
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
it "should call Slack API for commit comment events" do it "should call Slack API for commit comment events" do
data = Gitlab::NoteDataBuilder.build(commit_note, user) data = Gitlab::NoteDataBuilder.build(commit_note, user)
slack.execute(data) slack.execute(data)
expect(WebMock).to have_requested(:post, webhook_url).once expect(WebMock).to have_requested(:post, webhook_url).once
end end
end
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
it "should call Slack API for merge request comment events" do it "should call Slack API for merge request comment events" do
data = Gitlab::NoteDataBuilder.build(merge_request_note, user) data = Gitlab::NoteDataBuilder.build(merge_request_note, user)
...@@ -175,6 +183,12 @@ describe SlackService, models: true do ...@@ -175,6 +183,12 @@ describe SlackService, models: true do
expect(WebMock).to have_requested(:post, webhook_url).once expect(WebMock).to have_requested(:post, webhook_url).once
end end
end
context 'when issue comment event executed' do
let(:issue_note) do
create(:note_on_issue, project: project, note: "issue note")
end
it "should call Slack API for issue comment events" do it "should call Slack API for issue comment events" do
data = Gitlab::NoteDataBuilder.build(issue_note, user) data = Gitlab::NoteDataBuilder.build(issue_note, user)
...@@ -182,6 +196,13 @@ describe SlackService, models: true do ...@@ -182,6 +196,13 @@ describe SlackService, models: true do
expect(WebMock).to have_requested(:post, webhook_url).once expect(WebMock).to have_requested(:post, webhook_url).once
end end
end
context 'when snippet comment event executed' do
let(:snippet_note) do
create(:note_on_project_snippet, project: project,
note: "snippet note")
end
it "should call Slack API for snippet comment events" do it "should call Slack API for snippet comment events" do
data = Gitlab::NoteDataBuilder.build(snippet_note, user) data = Gitlab::NoteDataBuilder.build(snippet_note, user)
...@@ -190,4 +211,5 @@ describe SlackService, models: true do ...@@ -190,4 +211,5 @@ describe SlackService, models: true do
expect(WebMock).to have_requested(:post, webhook_url).once expect(WebMock).to have_requested(:post, webhook_url).once
end end
end end
end
end end
...@@ -258,8 +258,8 @@ describe API::API, api: true do ...@@ -258,8 +258,8 @@ describe API::API, api: true do
body: 'Hi!' body: 'Hi!'
end end
it 'responds with 500' do it 'responds with resource not found error' do
expect(response.status).to eq 500 expect(response.status).to eq 404
end end
it 'does not create new note' do it 'does not create new note' do
......
...@@ -2,7 +2,7 @@ require 'spec_helper' ...@@ -2,7 +2,7 @@ require 'spec_helper'
describe MergeRequests::MergeWhenBuildSucceedsService do describe MergeRequests::MergeWhenBuildSucceedsService do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:merge_request) { create(:merge_request) } let(:project) { create(:project) }
let(:mr_merge_if_green_enabled) do let(:mr_merge_if_green_enabled) do
create(:merge_request, merge_when_build_succeeds: true, merge_user: user, create(:merge_request, merge_when_build_succeeds: true, merge_user: user,
...@@ -10,11 +10,15 @@ describe MergeRequests::MergeWhenBuildSucceedsService do ...@@ -10,11 +10,15 @@ describe MergeRequests::MergeWhenBuildSucceedsService do
source_project: project, target_project: project, state: "opened") source_project: project, target_project: project, state: "opened")
end 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(: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') } let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, commit_message: 'Awesome message') }
describe "#execute" do 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 context 'first time enabling' do
before do before do
allow(merge_request).to receive(:ci_commit).and_return(ci_commit) allow(merge_request).to receive(:ci_commit).and_return(ci_commit)
......
...@@ -208,8 +208,10 @@ describe SystemNoteService, services: true do ...@@ -208,8 +208,10 @@ describe SystemNoteService, services: true do
end end
describe '.merge_when_build_succeeds' do describe '.merge_when_build_succeeds' do
let(:ci_commit) { build :ci_commit_without_jobs } let(:ci_commit) { build(:ci_commit_without_jobs )}
let(:noteable) { create :merge_request } 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) } subject { described_class.merge_when_build_succeeds(noteable, project, author, noteable.last_commit) }
...@@ -221,8 +223,10 @@ describe SystemNoteService, services: true do ...@@ -221,8 +223,10 @@ describe SystemNoteService, services: true do
end end
describe '.cancel_merge_when_build_succeeds' do describe '.cancel_merge_when_build_succeeds' do
let(:ci_commit) { build :ci_commit_without_jobs } let(:ci_commit) { build(:ci_commit_without_jobs) }
let(:noteable) { create :merge_request } let(:noteable) do
create(:merge_request, source_project: project, target_project: project)
end
subject { described_class.cancel_merge_when_build_succeeds(noteable, project, author) } subject { described_class.cancel_merge_when_build_succeeds(noteable, project, author) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment